Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

ResourceLoader: Have FileModule deliver additional path information

Browse source 
* Have FileModule::getScript() always return an array with a filePath
  or fakeFilePath, not a string. This allows source maps to be
  constructed.
* Make the scripts returned from Module::buildContent() always be an
  array. Module::getScript() may still return a string for b/c.
* In makeLoaderImplementScript(), interpret the new plainScripts array.
  Factor out the package file normalization loop from
  makeLoaderImplementScript().
* Fix missing base path in mediawiki.base.
* Improve relevant doc comments.

Bug: T47514
Change-Id: I392a8cce9a0febc707b6cb17412e3b723c9cc686
This commit is contained in:
Tim Starling 2023-07-24 15:37:04 +10:00 committed by Timo Tijhof
parent 1c90ec8019
commit 69ad795df7
9 changed files with 181 additions and 124 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

47
includes/ResourceLoader/FileModule.php
View file

@ -319,12 +319,6 @@ class FileModule extends Module {
return [ $localBasePath ?? MW_INSTALL_PATH, $remoteBasePath ];
}
/**
* Get all scripts for a given context concatenated together.
*
* @param Context $context Context in which to generate script
* @return string|array JavaScript code for $context, or package files data structure
*/
public function getScript( Context $context ) {
$packageFiles = $this->getPackageFiles( $context );
if ( $packageFiles !== null ) {
@ -338,7 +332,10 @@ class FileModule extends Module {
}
$files = $this->getScriptFiles( $context );
return $this->readScriptFiles( $context, $files );
foreach ( $files as &$file ) {
$this->readFileInfo( $context, $file );
}
return [ 'plainScripts' => $files ];
}
/**
@ -558,9 +555,8 @@ class FileModule extends Module {
// Add other configured paths
$scriptFileInfos = $this->getScriptFiles( $context );
foreach ( $scriptFileInfos as $fileInfo ) {
if ( isset( $fileInfo['filePath'] ) ) {
/** @var FilePath $filePath */
$filePath = $fileInfo['filePath'];
$filePath = $fileInfo['filePath'] ?? $fileInfo['versionFilePath'] ?? null;
if ( $filePath instanceof FilePath ) {
$files[] = $filePath->getLocalPath();
}
}
@ -952,23 +948,6 @@ class FileModule extends Module {
return $result;
}
/**
* Get the contents of a list of JavaScript files. Helper for getScript().
*
* @param Context $context
* @param array $scripts An array of file info arrays as returned by expandFileInfo()
* @return string Concatenated JavaScript code
*/
private function readScriptFiles( Context $context, array $scripts ) {
$js = '';
foreach ( $scripts as $fileInfo ) {
$this->readFileInfo( $context, $fileInfo );
// @phan-suppress-next-line PhanTypePossiblyInvalidDimOffset
$js .= ResourceLoader::ensureNewline( $fileInfo['content'] );
}
return $js;
}
/**
* Read the contents of a list of CSS files and remap and concatenate these.
*
@ -1270,7 +1249,14 @@ class FileModule extends Module {
* - type: (string) May be 'script', 'script-vue', 'data' or 'text'
* - filePath: (FilePath) The FilePath object which should be used to load the content.
* This will be absent if the content was loaded another way.
* - content: (string) The file contents, if known.
* - virtualFilePath: (FilePath) A FilePath object for a virtual path which doesn't actually
* exist. This is used for source map generation. Optional.
* - versionFilePath: (FilePath) A FilePath object which is the ultimate source of a
* generated file. The timestamp and contents will be used for version generation.
* Generated by the callback specified in versionCallback. Optional.
* - content: (string|mixed) If the 'type' element is 'script', this is a string containing
* JS code, being the contents of the script file. For any other type, this contains data
* which will be JSON serialized. Optional, if not set, it will be set in readFileInfo().
* - callback: (callable) A callback to call to obtain the contents. This will be set if the
* version callback was present in the input, indicating that the callback is expensive.
* - callbackParams: (array) The parameters to be passed to the callback.
@ -1346,7 +1332,7 @@ class FileModule extends Module {
);
if ( $callbackResult instanceof FilePath ) {
$callbackResult->initBasePaths( $this->localBasePath, $this->remoteBasePath );
$expanded['filePath'] = $callbackResult;
$expanded['versionFilePath'] = $callbackResult;
} else {
$expanded['definitionSummary'] = $callbackResult;
}
@ -1387,6 +1373,9 @@ class FileModule extends Module {
$this->getLogger()->error( $msg );
throw new LogicException( $msg );
}
if ( !isset( $expanded['filePath'] ) ) {
$expanded['virtualFilePath'] = $this->makeFilePath( $fileName );
}
return $expanded;
}

35
includes/ResourceLoader/Module.php
View file

@ -229,9 +229,8 @@ abstract class Module implements LoggerAwareInterface {
* Get all JS for this module for a given language and skin.
* Includes all relevant JS except loader scripts.
*
* For "plain" script modules, this should return a string with JS code. For multi-file modules
* where require() is used to load one file from another file, this should return an array
* structured as follows:
* For multi-file modules where require() is used to load one file from
* another file, this should return an array structured as follows:
* [
* 'files' => [
* 'file1.js' => [ 'type' => 'script', 'content' => 'JS code' ],
@ -241,9 +240,28 @@ abstract class Module implements LoggerAwareInterface {
* 'main' => 'file1.js'
* ]
*
* For plain concatenated scripts, this can either return a string, or an
* associative array similar to the one used for package files:
* [
* 'plainScripts' => [
* [ 'content' => 'JS code' ],
* [ 'content' => 'JS code' ],
* ],
* ]
*
* @stable to override
* @param Context $context
* @return string|array JavaScript code (string), or multi-file structure described above (array)
* @return string|array JavaScript code (string), or multi-file array with the
* following keys:
* - files: An associative array mapping file name to file info structure
* - main: The name of the main script, a key in the files array
* - plainScripts: An array of file info structures to be concatenated and
* executed when the module is loaded.
* Each file info structure has the following keys:
* - type: May be "script", "script-vue" or "data". Optional, default "script".
* - content: The string content of the file
* - filePath: A FilePath object describing the location of the source file.
* This will be used to construct the source map during minification.
*/
public function getScript( Context $context ) {
// Stub, override expected
@ -830,15 +848,8 @@ abstract class Module implements LoggerAwareInterface {
$scripts = $this->getScriptURLsForDebug( $context );
} else {
$scripts = $this->getScript( $context );
// Make the script safe to concatenate by making sure there is at least one
// trailing new line at the end of the content. Previously, this looked for
// a semi-colon instead, but that breaks concatenation if the semicolon
// is inside a comment like "// foo();". Instead, simply use a
// line break as separator which matches JavaScript native logic for implicitly
// ending statements even if a semi-colon is missing.
// Bugs: T29054, T162719.
if ( is_string( $scripts ) ) {
$scripts = ResourceLoader::ensureNewline( $scripts );
$scripts = [ 'plainScripts' => [ [ 'content' => $scripts ] ] ];
}
}
$content['scripts'] = $scripts;

139
includes/ResourceLoader/ResourceLoader.php
View file

@ -1091,12 +1091,17 @@ MESSAGE;
switch ( $only ) {
case 'scripts':
$scripts = $content['scripts'];
if ( is_string( $scripts ) ) {
// Load scripts raw...
$strContent = $scripts;
} elseif ( is_array( $scripts ) ) {
// ...except when $scripts is an array of URLs or an associative array
$strContent = self::makeLoaderImplementScript(
if ( !is_array( $scripts ) ) {
// Formerly scripts was usually a string, but now it is
// normalized to an array by buildContent().
throw new InvalidArgumentException( 'scripts must be an array' );
}
if ( isset( $scripts['plainScripts'] ) ) {
// Add plain scripts
$strContent .= self::concatenatePlainScripts( $scripts['plainScripts'] );
} elseif ( isset( $scripts['files'] ) ) {
// Add implement call if any
$strContent .= self::makeLoaderImplementScript(
$implementKey,
$scripts,
[],
@ -1115,20 +1120,19 @@ MESSAGE;
break;
default:
$scripts = $content['scripts'] ?? '';
if ( is_string( $scripts ) ) {
if ( $name === 'site' || $name === 'user' ) {
// Legacy scripts that run in the global scope without a closure.
// mw.loader.impl will use eval if scripts is a string.
// Minify manually here, because general response minification is
// not effective due it being a string literal, not a function.
if ( !$debug ) {
$scripts = self::filter( 'minify-js', $scripts ); // T107377
}
} else {
$scripts = new HtmlJsCode( $scripts );
if ( ( $name === 'site' || $name === 'user' )
&& isset( $scripts['plainScripts'] )
) {
// Legacy scripts that run in the global scope without a closure.
// mw.loader.impl will use eval if scripts is a string.
// Minify manually here, because general response minification is
// not effective due it being a string literal, not a function.
$scripts = self::concatenatePlainScripts( $scripts['plainScripts'] );
if ( !$debug ) {
$scripts = self::filter( 'minify-js', $scripts ); // T107377
}
}
$strContent = self::makeLoaderImplementScript(
$strContent .= self::makeLoaderImplementScript(
$implementKey,
$scripts,
$content['styles'] ?? [],
@ -1194,9 +1198,8 @@ MESSAGE;
* Return JS code that calls mw.loader.impl with given module properties.
*
* @param string $name Module name used as implement key (format "`[name]@[version]`")
* @param HtmlJsCode|array|string|string[] $scripts
* - HtmlJsCode: Concatenated scripts to be wrapped in a closure
* - array: Package files array containing HtmlJsCode for individual JS files,
* @param array|string|string[] $scripts
* - array: Package files array containing strings for individual JS files,
* as produced by Module::getScript().
* - string: Script contents to eval in global scope (for site/user scripts).
* - string[]: List of URLs (for debug mode).
@ -1213,35 +1216,31 @@ MESSAGE;
private static function makeLoaderImplementScript(
$name, $scripts, $styles, $messages, $templates, $deprecationWarning
) {
if ( $scripts instanceof HtmlJsCode ) {
if ( $scripts->value === '' ) {
$scripts = null;
} else {
$scripts = new HtmlJsCode( "function ( $, jQuery, require, module ) {\n{$scripts->value}\n}" );
}
} elseif ( is_array( $scripts ) && isset( $scripts['files'] ) ) {
$files = $scripts['files'];
foreach ( $files as &$file ) {
// $file is changed (by reference) from a descriptor array to the content of the file
// All of these essentially do $file = $file['content'];, some just have wrapping around it
if ( $file['type'] === 'script' ) {
// Ensure that the script has a newline at the end to close any comment in the
// last line.
$content = self::ensureNewline( $file['content'] );
// Provide CJS `exports` (in addition to CJS2 `module.exports`) to package modules (T284511).
// $/jQuery are simply used as globals instead.
// TODO: Remove $/jQuery param from traditional module closure too (and bump caching)
$file = new HtmlJsCode( "function ( require, module, exports ) {\n$content}" );
if ( is_string( $scripts ) ) {
// user/site script
} elseif ( is_array( $scripts ) ) {
if ( isset( $scripts['files'] ) ) {
$files = self::encodeFiles( $scripts['files'] );
$scripts = HtmlJsCode::encodeObject( [
'main' => $scripts['main'],
'files' => HtmlJsCode::encodeObject( $files, true )
], true );
} elseif ( isset( $scripts['plainScripts'] ) ) {
$plainScripts = self::concatenatePlainScripts( $scripts['plainScripts'] );
if ( $plainScripts === '' ) {
$scripts = null;
} else {
$file = $file['content'];
$scripts = new HtmlJsCode(
"function ( $, jQuery, require, module ) {\n{$plainScripts}}" );
}
} elseif ( $scripts === [] || isset( $scripts[0] ) ) {
// Array of URLs
} else {
throw new InvalidArgumentException( 'Invalid script array: ' .
'must contain files, plainScripts or be an array of URLs' );
}
$scripts = HtmlJsCode::encodeObject( [
'main' => $scripts['main'],
'files' => HtmlJsCode::encodeObject( $files, true )
], true );
} elseif ( !is_string( $scripts ) && !is_array( $scripts ) ) {
throw new InvalidArgumentException( 'Script must be a string or an array of URLs' );
} else {
throw new InvalidArgumentException( 'Script must be a string or array' );
}
// mw.loader.impl requires 'styles', 'messages' and 'templates' to be objects (not
@ -1265,6 +1264,52 @@ MESSAGE;
Html::encodeJsList( $module, true ) . '];});';
}
/**
* Extract the contents of an array of package files, and convert it to an
* array of data which can be passed to HtmlJsCode::encodeObject(), with any
* JS code wrapped in HtmlJsCode objects.
*
* Package files can contain JSON data.
*
* @param array $files
* @return array
*/
private static function encodeFiles( $files ) {
foreach ( $files as &$file ) {
// $file is changed (by reference) from a descriptor array to the content of the file
// All of these essentially do $file = $file['content'];, some just have wrapping around it
if ( $file['type'] === 'script' ) {
// Ensure that the script has a newline at the end to close any comment in the
// last line.
$content = self::ensureNewline( $file['content'] );
// Provide CJS `exports` (in addition to CJS2 `module.exports`) to package modules (T284511).
// $/jQuery are simply used as globals instead.
// TODO: Remove $/jQuery param from traditional module closure too (and bump caching)
$file = new HtmlJsCode( "function ( require, module, exports ) {\n$content}" );
} else {
$file = $file['content'];
}
}
return $files;
}
/**
* Combine a plainScripts array like [ [ 'content' => '...' ] ] into a
* single string.
*
* @param array[] $plainScripts
* @return string
*/
private static function concatenatePlainScripts( $plainScripts ) {
$s = '';
foreach ( $plainScripts as $script ) {
// Make the script safe to concatenate by making sure there is at least one
// trailing new line at the end of the content (T29054, T162719)
$s .= self::ensureNewline( $script['content'] );
}
return $s;
}
/**
* Combines an associative array mapping media type to CSS into a
* single stylesheet with "@media" blocks.

3
resources/Resources.php
View file

@ -133,6 +133,7 @@ return [
],
'mediawiki.base' => [
'localBasePath' => MW_INSTALL_PATH . '/resources/src/mediawiki.base',
'remoteBasePath' => "$wgResourceBasePath/resources/src/mediawiki.base",
'packageFiles' => [
// This MUST be kept in sync with maintenance/jsduck/eg-iframe.html
'mediawiki.base.js',
@ -620,7 +621,7 @@ return [
],
'dependencies' => [
'vue'
]
],
],
'@wikimedia/codex' => [

4
tests/phpunit/ResourceLoaderTestCase.php
View file

@ -13,10 +13,10 @@ abstract class ResourceLoaderTestCase extends MediaWikiIntegrationTestCase {
// Version hash for a blank file module.
// Result of ResourceLoader::makeHash(), ResourceLoaderTestModule
// and FileModule::getDefinitionSummary().
public const BLANK_VERSION = '9p30q';
public const BLANK_VERSION = 'dukpe';
// Result of ResourceLoader::makeVersionQuery() for a blank file module.
// In other words, result of ResourceLoader::makeHash( BLANK_VERSION );
public const BLANK_COMBI = 'rbml8';
public const BLANK_COMBI = '1xz0a';
/**
* @param array|string $options Language code or options array

2
tests/phpunit/includes/OutputPageTest.php
View file

@ -2599,7 +2599,7 @@ class OutputPageTest extends MediaWikiIntegrationTestCase {
[
[ 'test.quux', RL\Module::TYPE_COMBINED ],
"<script>(RLQ=window.RLQ||[]).push(function(){"
. "mw.loader.impl(function(){return[\"test.quux@1ev0i\",function($,jQuery,require,module){"
. "mw.loader.impl(function(){return[\"test.quux@1b4i1\",function($,jQuery,require,module){"
. "mw.test.baz({token:123});},{\"css\":[\".mw-icon{transition:none}"
. "\"]}];});});</script>"
],

61
tests/phpunit/includes/ResourceLoader/FileModuleTest.php
View file

@ -121,32 +121,41 @@ class FileModuleTest extends ResourceLoaderTestCase {
}
public function testGetScript() {
$localBasePath = __DIR__ . '/../../data/resourceloader';
$remoteBasePath = '/w';
$module = new FileModule( [
'localBasePath' => __DIR__ . '/../../data/resourceloader',
'localBasePath' => $localBasePath,
'remoteBasePath' => $remoteBasePath,
'scripts' => [ 'script-nosemi.js', 'script-comment.js' ],
] );
$module->setName( 'testing' );
$ctx = $this->getResourceLoaderContext();
$this->assertEquals(
"/* eslint-disable */\nmw.foo()\n" .
"/* eslint-disable */\nmw.foo()\n// mw.bar();\n",
$module->getScript( $ctx ),
'scripts with newline at the end are concatenated without a newline'
);
$module = new FileModule( [
'localBasePath' => __DIR__ . '/../../data/resourceloader',
'scripts' => [ 'script-nosemi-nonl.js', 'script-comment-nonl.js' ],
] );
$module->setName( 'testing' );
$ctx = $this->getResourceLoaderContext();
$this->assertEquals(
"/* eslint-disable */\nmw.foo()" .
"\n" .
"/* eslint-disable */\nmw.foo()\n// mw.bar();" .
"\n",
$module->getScript( $ctx ),
'scripts without newline at the end are concatenated with a newline'
[
'plainScripts' => [
'script-nosemi.js' => [
'name' => 'script-nosemi.js',
'content' => "/* eslint-disable */\nmw.foo()\n",
'type' => 'script',
'filePath' => new FilePath(
'script-nosemi.js',
$localBasePath,
$remoteBasePath
)
],
'script-comment.js' => [
'name' => 'script-comment.js',
'content' => "/* eslint-disable */\nmw.foo()\n// mw.bar();\n",
'type' => 'script',
'filePath' => new FilePath(
'script-comment.js',
$localBasePath,
$remoteBasePath
)
]
]
],
$module->getScript( $ctx )
);
}
@ -648,6 +657,7 @@ class FileModuleTest extends ResourceLoaderTestCase {
'foo.json' => [
'type' => 'data',
'content' => [ 'Hello' => 'world' ],
'virtualFilePath' => 'foo.json',
],
'sample.json' => [
'type' => 'data',
@ -657,17 +667,20 @@ class FileModuleTest extends ResourceLoaderTestCase {
'bar.js' => [
'type' => 'script',
'content' => "console.log('Hello');",
'virtualFilePath' => 'bar.js',
],
'data.json' => [
'type' => 'data',
'content' => [ 'langCode' => 'fy', 'extra' => [ 'a' => 'b' ] ],
'virtualFilePath' => 'data.json',
],
'config.json' => [
'type' => 'data',
'content' => [
'Sitename' => $config->get( MainConfigNames::Sitename ),
'server' => $config->get( MainConfigNames::ServerName ),
]
],
'virtualFilePath' => 'config.json',
]
],
'main' => 'bar.js'
@ -697,10 +710,12 @@ class FileModuleTest extends ResourceLoaderTestCase {
'bar.js' => [
'type' => 'script',
'content' => "console.log('Hello');",
'virtualFilePath' => 'bar.js',
],
'data.json' => [
'type' => 'data',
'content' => [ 'langCode' => 'fy', 'extra' => [ 'A', 'B' ] ],
'virtualFilePath' => 'data.json',
],
],
'main' => 'bar.js'
@ -901,6 +916,10 @@ class FileModuleTest extends ResourceLoaderTestCase {
$this->assertInstanceOf( FilePath::class, $file['filePath'] );
$file['filePath'] = $file['filePath']->getPath();
}
if ( isset( $file['virtualFilePath'] ) ) {
$this->assertInstanceOf( FilePath::class, $file['virtualFilePath'] );
$file['virtualFilePath'] = $file['virtualFilePath']->getPath();
}
}
// Check the rest of the result
$this->assertEquals( $expected, $result );

11
tests/phpunit/includes/ResourceLoader/ModuleTest.php
View file

@ -193,31 +193,24 @@ class ModuleTest extends ResourceLoaderTestCase {
return [
[
"mw.foo()",
"mw.foo()\n",
],
[
"mw.foo();",
"mw.foo();\n",
],
[
"mw.foo();\n",
"mw.foo();\n",
],
[
"mw.foo()\n",
"mw.foo()\n",
],
[
"mw.foo()\n// mw.bar();",
"mw.foo()\n// mw.bar();\n",
],
[
"mw.foo()\n// mw.bar()",
"mw.foo()\n// mw.bar()\n",
],
[
"mw.foo()// mw.bar();",
"mw.foo()// mw.bar();\n",
],
];
}
@ -225,7 +218,7 @@ class ModuleTest extends ResourceLoaderTestCase {
/**
* @dataProvider provideBuildContentScripts
*/
public function testBuildContentScripts( $raw, $build, $message = '' ) {
public function testBuildContentScripts( $raw, $message = '' ) {
$context = $this->getResourceLoaderContext();
$module = new ResourceLoaderTestModule( [
'script' => $raw
@ -233,7 +226,7 @@ class ModuleTest extends ResourceLoaderTestCase {
$module->setName( "" );
$this->assertEquals( $raw, $module->getScript( $context ), 'Raw script' );
$this->assertEquals(
$build,
[ 'plainScripts' => [ [ 'content' => $raw ] ] ],
$module->getModuleContent( $context )[ 'scripts' ],
$message
);

3
tests/phpunit/includes/ResourceLoader/ResourceLoaderTest.php
View file

@ -440,7 +440,6 @@ mw.example();
'expected' => 'mw.loader.impl(function(){return[ "test.example", function ( $, jQuery, require, module ) {
mw.example();
} ];});',
] ],
[ [
@ -575,7 +574,7 @@ END
$rl->makeLoaderImplementScript(
$case['name'],
( $case['wrap'] && is_string( $case['scripts'] ) )
? new HtmlJsCode( $case['scripts'] )
? [ 'plainScripts' => [ [ 'content' => $case['scripts'] ] ] ]
: $case['scripts'],
$case['styles'],
$case['messages'],