Merge "resourceloader: Fix mw.loader to compute version for current request only"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Wed, 14 Mar 2018 21:49:50 +0000 (21:49 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Wed, 14 Mar 2018 21:49:50 +0000 (21:49 +0000)
resources/src/mediawiki/mediawiki.js
tests/qunit/data/load.mock.php
tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js

index aa93ca2..450d4ff 100644 (file)
                         *
                         * @private
                         * @param {string} moduleStr Module list for load.php `module` query parameter
-                        * @param {Object} currReqBase Object with other parameters (other than 'modules') to use in the request
+                        * @param {Array} modules List of module names (not packed)
+                        * @param {Object} currReqBase Object with base parameters for this request
                         * @param {string} sourceLoadScript URL of load.php
                         */
-                       function doRequest( moduleStr, currReqBase, sourceLoadScript ) {
+                       function doRequest( moduleStr, modules, currReqBase, sourceLoadScript ) {
                                // Optimisation: Inherit (Object.create), not copy ($.extend)
                                var query = Object.create( currReqBase );
                                query.modules = moduleStr;
+                               query.version = getCombinedVersion( modules );
                                query = sortQuery( query );
                                addScript( sourceLoadScript + '?' + $.param( query ) );
                        }
                        function batchRequest( batch ) {
                                var reqBase, splits, maxQueryLength, b, bSource, bGroup, bSourceGroup,
                                        source, group, i, modules, sourceLoadScript,
-                                       currReqBase, currReqBaseLength, moduleMap, l,
+                                       currReqBase, currReqBaseLength, moduleMap, currReqModules, l,
                                        lastDotIndex, prefix, suffix, bytesAdded;
 
                                if ( !batch.length ) {
                                // misses for otherwise identical content.
                                batch.sort();
 
-                               // Build a list of query parameters common to all requests
+                               // Query parameters common to all requests
                                reqBase = {
                                        skin: mw.config.get( 'skin' ),
                                        lang: mw.config.get( 'wgUserLanguage' ),
                                                // modules for this group from this source.
                                                modules = splits[ source ][ group ];
 
+                                               // Query parameters common to requests for this module group
                                                // Optimisation: Inherit (Object.create), not copy ($.extend)
                                                currReqBase = Object.create( reqBase );
-                                               currReqBase.version = getCombinedVersion( modules );
-
-                                               // For user modules append a user name to the query string.
+                                               // User modules require a user name in the query string.
                                                if ( group === 'user' && mw.config.get( 'wgUserName' ) !== null ) {
                                                        currReqBase.user = mw.config.get( 'wgUserName' );
                                                }
-                                               currReqBaseLength = $.param( currReqBase ).length;
+
+                                               // In addition to currReqBase, doRequest() will also add 'modules' and 'version'.
+                                               // > '&modules='.length === 9
+                                               // > '&version=1234567'.length === 16
+                                               // > 9 + 16 = 25
+                                               currReqBaseLength = $.param( currReqBase ).length + 25;
+
                                                // We may need to split up the request to honor the query string length limit,
                                                // so build it piece by piece.
-                                               l = currReqBaseLength + 9; // '&modules='.length == 9
-
+                                               l = currReqBaseLength;
                                                moduleMap = {}; // { prefix: [ suffixes ] }
+                                               currReqModules = [];
 
                                                for ( i = 0; i < modules.length; i++ ) {
                                                        // Determine how many bytes this module would add to the query string
                                                        lastDotIndex = modules[ i ].lastIndexOf( '.' );
-
                                                        // If lastDotIndex is -1, substr() returns an empty string
                                                        prefix = modules[ i ].substr( 0, lastDotIndex );
                                                        suffix = modules[ i ].slice( lastDotIndex + 1 );
-
                                                        bytesAdded = hasOwn.call( moduleMap, prefix ) ?
                                                                suffix.length + 3 : // '%2C'.length == 3
                                                                modules[ i ].length + 3; // '%7C'.length == 3
 
-                                                       // If the url would become too long, create a new one,
-                                                       // but don't create empty requests
-                                                       if ( maxQueryLength > 0 && !$.isEmptyObject( moduleMap ) && l + bytesAdded > maxQueryLength ) {
-                                                               // This url would become too long, create a new one, and start the old one
-                                                               doRequest( buildModulesString( moduleMap ), currReqBase, sourceLoadScript );
+                                                       // If the url would become too long, create a new one, but don't create empty requests
+                                                       if ( maxQueryLength > 0 && currReqModules.length && l + bytesAdded > maxQueryLength ) {
+                                                               // Dispatch what we've got...
+                                                               doRequest( buildModulesString( moduleMap ), currReqModules, currReqBase, sourceLoadScript );
+                                                               // .. and start again.
+                                                               l = currReqBaseLength;
                                                                moduleMap = {};
-                                                               l = currReqBaseLength + 9;
+                                                               currReqModules = [];
+
                                                                mw.track( 'resourceloader.splitRequest', { maxQueryLength: maxQueryLength } );
                                                        }
                                                        if ( !hasOwn.call( moduleMap, prefix ) ) {
                                                                moduleMap[ prefix ] = [];
                                                        }
-                                                       moduleMap[ prefix ].push( suffix );
                                                        l += bytesAdded;
+                                                       moduleMap[ prefix ].push( suffix );
+                                                       currReqModules.push( modules[ i ] );
                                                }
                                                // If there's anything left in moduleMap, request that too
-                                               if ( !$.isEmptyObject( moduleMap ) ) {
-                                                       doRequest( buildModulesString( moduleMap ), currReqBase, sourceLoadScript );
+                                               if ( currReqModules.length ) {
+                                                       doRequest( buildModulesString( moduleMap ), currReqModules, currReqBase, sourceLoadScript );
                                                }
                                        }
                                }
index fed5746..43ee0c7 100644 (file)
@@ -49,18 +49,45 @@ mw.loader.implement( 'testNotSkipped', function () {}, {}, {});
 
        'testUsesSkippable' => "
 mw.loader.implement( 'testUsesSkippable', function () {}, {}, {});
+",
+
+       'testUrlInc' => "
+mw.loader.implement( 'testUrlInc', function () {} );
+",
+       'testUrlInc.a' => "
+mw.loader.implement( 'testUrlInc.a', function () {} );
+",
+       'testUrlInc.b' => "
+mw.loader.implement( 'testUrlInc.b', function () {} );
 ",
 ];
 
 $response = '';
 
-// Only support for non-encoded module names, full module names expected
+// Does not support the full behaviour of ResourceLoaderContext::expandModuleNames(),
+// Only supports dotless module names joined by comma,
+// with the exception of the hardcoded cases for testUrl*.
 if ( isset( $_GET['modules'] ) ) {
-       $modules = explode( ',', $_GET['modules'] );
+       if ( $_GET['modules'] === 'testUrlInc,testUrlIncDump|testUrlInc.a,b' ) {
+               $modules = [ 'testUrlInc', 'testUrlIncDump', 'testUrlInc.a', 'testUrlInc.b' ];
+       } else {
+               $modules = explode( ',', $_GET['modules'] );
+       }
        foreach ( $modules as $module ) {
                if ( isset( $moduleImplementations[$module] ) ) {
                        $response .= $moduleImplementations[$module];
+               } elseif ( preg_match( '/^test.*Dump$/', $module ) === 1 ) {
+                       $queryModules = $_GET['modules'];
+                       $queryVersion = isset( $_GET['version'] ) ? strval( $_GET['version'] ) : null;
+                       $response .= 'mw.loader.implement( ' . json_encode( $module )
+                               . ', function ( $, jQuery, require, module ) {'
+                               . 'module.exports.query = { '
+                               . 'modules: ' . json_encode( $queryModules ) . ','
+                               . 'version: ' . json_encode( $queryVersion )
+                               . ' };'
+                               . '} );';
                } else {
+                       // Default
                        $response .= 'mw.loader.state(' . json_encode( $module ) . ', "missing" );' . "\n";
                }
        }
index e774112..6c988ef 100644 (file)
                assert.strictEqual( mw.loader.getState( 'test.empty' ), 'ready' );
        } );
 
+       // @covers mw.loader#batchRequest
+       // This is a regression test because in the past we called getCombinedVersion()
+       // for all requested modules, before url splitting took place.
+       // Discovered as part of T188076, but not directly related.
+       QUnit.test( 'Url composition (modules considered for version)', function ( assert ) {
+               mw.loader.register( [
+                       // [module, version, dependencies, group, source]
+                       [ 'testUrlInc', 'url', [], null, 'testloader' ],
+                       [ 'testUrlIncDump', 'dump', [], null, 'testloader' ]
+               ] );
+
+               mw.config.set( 'wgResourceLoaderMaxQueryLength', 10 );
+
+               return mw.loader.using( [ 'testUrlIncDump', 'testUrlInc' ] ).then( function ( require ) {
+                       assert.propEqual(
+                               require( 'testUrlIncDump' ).query,
+                               {
+                                       modules: 'testUrlIncDump',
+                                       // Expected: Wrapped hash just for this one module
+                                       //   $hash = hash( 'fnv132', 'dump');
+                                       //   base_convert( $hash, 16, 36 ); // "13e9zzn"
+                                       // Previously: Wrapped hash for both modules, despite being in separate requests
+                                       //   $hash = hash( 'fnv132', 'urldump' );
+                                       //   base_convert( $hash, 16, 36 ); // "18kz9ca"
+                                       version: '13e9zzn'
+                               },
+                               'Query parameters'
+                       );
+
+                       assert.strictEqual( mw.loader.getState( 'testUrlInc' ), 'ready', 'testUrlInc also loaded' );
+               } );
+       } );
+
        QUnit.test( 'Broken indirect dependency', function ( assert ) {
                // don't emit an error event
                this.sandbox.stub( mw, 'track' );