resourceloader: Optimise mw.loader.register()
authorTimo Tijhof <krinklemail@gmail.com>
Fri, 20 Nov 2015 01:14:09 +0000 (01:14 +0000)
committerOri.livneh <ori@wikimedia.org>
Tue, 24 Nov 2015 20:28:45 +0000 (20:28 +0000)
* Remove redundant 'len' variables. This is a dated micro-optimisation
  that is no longer useful and can actually have an adverse effect.

* Remove [.length] assignment. Use native push() instead.

* Remove JSLint-ism of "i += 1" instead of "i++".

* Remove redundant 'delete' operation for local 'unresolved'
  object. This falls naturally out of scope. Mutating this object
  to save memory probably wastes more cycles than it saves.

Change-Id: Ib3cb40d2fef696078bd64585db9498326f08b2d2

resources/src/mediawiki/mediawiki.js
tests/qunit/suites/resources/mediawiki/mediawiki.test.js

index 293fd58..322c579 100644 (file)
                 */
                params: function ( parameters ) {
                        var i;
-                       for ( i = 0; i < parameters.length; i += 1 ) {
+                       for ( i = 0; i < parameters.length; i++ ) {
                                this.parameters.push( parameters[ i ] );
                        }
                        return this;
                                }
 
                                // Execute all jobs whose dependencies are either all satisfied or contain at least one failed module.
-                               for ( j = 0; j < jobs.length; j += 1 ) {
+                               for ( j = 0; j < jobs.length; j++ ) {
                                        hasErrors = anyFailed( jobs[ j ].dependencies );
                                        if ( hasErrors || allReady( jobs[ j ].dependencies ) ) {
                                                // All dependencies satisfied, or some have errors
                         * @throws {Error} If any unregistered module or a dependency loop is encountered
                         */
                        function sortDependencies( module, resolved, unresolved ) {
-                               var n, deps, len, skip;
+                               var i, deps, skip;
 
                                if ( !hasOwn.call( registry, module ) ) {
                                        throw new Error( 'Unknown dependency: ' + module );
                                }
                                // Tracks down dependencies
                                deps = registry[ module ].dependencies;
-                               len = deps.length;
-                               for ( n = 0; n < len; n += 1 ) {
-                                       if ( $.inArray( deps[ n ], resolved ) === -1 ) {
-                                               if ( unresolved[ deps[ n ] ] ) {
-                                                       throw new Error(
-                                                               'Circular reference detected: ' + module +
-                                                               ' -> ' + deps[ n ]
-                                                       );
+                               for ( i = 0; i < deps.length; i++ ) {
+                                       if ( $.inArray( deps[ i ], resolved ) === -1 ) {
+                                               if ( unresolved[ deps[ i ] ] ) {
+                                                       throw new Error( mw.format(
+                                                               'Circular reference detected: $1 -> $2',
+                                                               module,
+                                                               deps[ i ]
+                                                       ) );
                                                }
 
                                                // Add to unresolved
                                                unresolved[ module ] = true;
-                                               sortDependencies( deps[ n ], resolved, unresolved );
-                                               delete unresolved[ module ];
+                                               sortDependencies( deps[ i ], resolved, unresolved );
                                        }
                                }
-                               resolved[ resolved.length ] = module;
+                               resolved.push( module );
                        }
 
                        /**
-                        * Get a list of module names that a module depends on in their proper dependency
-                        * order.
+                        * Get names of module that a module depends on, in their proper dependency order.
                         *
                         * @private
                         * @param {string[]} modules Array of string module names
                        }
 
                        /**
-                        * Load and execute a script with callback.
+                        * Load and execute a script.
                         *
                         * @private
                         * @param {string} src URL to script, will be used as the src attribute in the script tag
                                                // Array of css strings in key 'css',
                                                // or back-compat array of urls from media-type
                                                if ( $.isArray( value ) ) {
-                                                       for ( i = 0; i < value.length; i += 1 ) {
+                                                       for ( i = 0; i < value.length; i++ ) {
                                                                if ( key === 'bc-url' ) {
                                                                        // back-compat: { <media>: [url, ..] }
                                                                        addLink( media, value[ i ] );
                                                        // { "url": { <media>: [url, ..] } }
                                                        for ( media in value ) {
                                                                urls = value[ media ];
-                                                               for ( i = 0; i < urls.length; i += 1 ) {
+                                                               for ( i = 0; i < urls.length; i++ ) {
                                                                        addLink( media, urls[ i ] );
                                                                }
                                                        }
                                        }
                                }
                                a.sort();
-                               for ( key = 0; key < a.length; key += 1 ) {
+                               for ( key = 0; key < a.length; key++ ) {
                                        sorted[ a[ key ] ] = o[ a[ key ] ];
                                }
                                return sorted;
                                        maxQueryLength = mw.config.get( 'wgResourceLoaderMaxQueryLength', 2000 );
 
                                        // Appends a list of modules from the queue to the batch
-                                       for ( q = 0; q < queue.length; q += 1 ) {
+                                       for ( q = 0; q < queue.length; q++ ) {
                                                // Only request modules which are registered
                                                if ( hasOwn.call( registry, queue[ q ] ) && registry[ queue[ q ] ].state === 'registered' ) {
                                                        // Prevent duplicate entries
                                                        if ( $.inArray( queue[ q ], batch ) === -1 ) {
-                                                               batch[ batch.length ] = queue[ q ];
+                                                               batch.push( queue[ q ] );
                                                                // Mark registered modules as loading
                                                                registry[ queue[ q ] ].state = 'loading';
                                                        }
                                        batch.sort();
 
                                        // Split batch by source and by group.
-                                       for ( b = 0; b < batch.length; b += 1 ) {
+                                       for ( b = 0; b < batch.length; b++ ) {
                                                bSource = registry[ batch[ b ] ].source;
                                                bGroup = registry[ batch[ b ] ].group;
                                                if ( !hasOwn.call( splits, bSource ) ) {
                                                        splits[ bSource ][ bGroup ] = [];
                                                }
                                                bSourceGroup = splits[ bSource ][ bGroup ];
-                                               bSourceGroup[ bSourceGroup.length ] = batch[ b ];
+                                               bSourceGroup.push( batch[ b ] );
                                        }
 
                                        // Clear the batch - this MUST happen before we append any
 
                                                        moduleMap = {}; // { prefix: [ suffixes ] }
 
-                                                       for ( i = 0; i < modules.length; i += 1 ) {
+                                                       for ( i = 0; i < modules.length; i++ ) {
                                                                // Determine how many bytes this module would add to the query string
                                                                lastDotIndex = modules[ i ].lastIndexOf( '.' );
 
                                 * @param {string} [skip=null] Script body of the skip function
                                 */
                                register: function ( module, version, dependencies, group, source, skip ) {
-                                       var i, len;
+                                       var i;
                                        // Allow multiple registration
                                        if ( typeof module === 'object' ) {
                                                resolveIndexedDependencies( module );
-                                               for ( i = 0, len = module.length; i < len; i++ ) {
+                                               for ( i = 0; i < module.length; i++ ) {
                                                        // module is an array of module names
                                                        if ( typeof module[ i ] === 'string' ) {
                                                                mw.loader.register( module[ i ] );
index d205ba7..bc25ce0 100644 (file)
                } );
        } );
 
-       QUnit.asyncTest( 'mw.loader.using( .. ).promise', 2, function ( assert ) {
+       QUnit.asyncTest( 'mw.loader.using( .. ) Promise', 2, function ( assert ) {
                var isAwesomeDone;
 
                mw.loader.testCallback = function () {
        } );
 
        // @import (bug 31676)
-       QUnit.asyncTest( 'mw.loader.implement( styles has @import)', 7, function ( assert ) {
+       QUnit.asyncTest( 'mw.loader.implement( styles has @import )', 7, function ( assert ) {
                var isJsExecuted, $element;
 
                mw.loader.implement(
                } );
        } );
 
-       QUnit.test( 'mw.loader erroneous indirect dependency', 4, function ( assert ) {
+       QUnit.test( 'mw.loader with broken indirect dependency', 4, function ( assert ) {
                // don't emit an error event
                this.sandbox.stub( mw, 'track' );
 
                assert.strictEqual( mw.track.callCount, 1 );
        } );
 
+       QUnit.test( 'mw.loader with circular dependency', 1, function ( assert ) {
+               mw.loader.register( [
+                       [ 'test.circle1', '0', [ 'test.circle2' ] ],
+                       [ 'test.circle2', '0', [ 'test.circle3' ] ],
+                       [ 'test.circle3', '0', [ 'test.circle1' ] ]
+               ] );
+               assert.throws( function () {
+                       mw.loader.using( 'test.circle3' );
+               }, /Circular/, 'Detect circular dependency' );
+       } );
+
        QUnit.test( 'mw.loader out-of-order implementation', 9, function ( assert ) {
                mw.loader.register( [
                        [ 'test.module4', '0' ],