resourceloader: Move add() outside loop to optimise sortDependencies()

Follows-up CR of dec800968.

As being a set, repeatedly calling unresolved.add() with the same
module name is not useful. This removes that needless overhead
by moving the statement from inside the loop to before it.

This does mean it is now before the first call to has(), but this
change does not affect its behaviour. Tests confirm that.

Aside from optimising the loop, this also ends up reducing the level
of recursion required to detect self-dependencies (eg. A > A).

At first I thought that self-dependencies were not detectable
right now, but the recursion made this work previously as well.
I've added a test case to confirm this going forward, and updated
the existing test cases to consistently use ABC in examples.

Change-Id: I9f4d0a18750f8e5778e0bf3c693b1d83a4ec4312

diff --git a/resources/src/startup/mediawiki.js b/resources/src/startup/mediawiki.js
index a2ba85f..63d23d8 100644 (file)
--- a/resources/src/startup/mediawiki.js
+++ b/resources/src/startup/mediawiki.js
@@ -920,6 +920,7 @@
 
                                // Tracks down dependencies
                                deps = registry[ module ].dependencies;
+                               unresolved.add( module );
                                for ( i = 0; i < deps.length; i++ ) {
                                        if ( resolved.indexOf( deps[ i ] ) === -1 ) {
                                                if ( unresolved.has( deps[ i ] ) ) {
@@ -928,7 +929,6 @@
                                                        );
                                                }
 
-                                               unresolved.add( module );
                                                sortDependencies( deps[ i ], resolved, unresolved );
                                        }
                                }

diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js
index 6b2738b..05b85be 100644 (file)
--- a/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js
+++ b/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js
@@ -124,11 +124,11 @@
                var done = assert.async();
 
                mw.loader.register( [
-                       [ 'test.circle1', '0', [ 'test.circle2' ] ],
-                       [ 'test.circle2', '0', [ 'test.circle3' ] ],
-                       [ 'test.circle3', '0', [ 'test.circle1' ] ]
+                       [ 'test.set.circleA', '0', [ 'test.set.circleB' ] ],
+                       [ 'test.set.circleB', '0', [ 'test.set.circleC' ] ],
+                       [ 'test.set.circleC', '0', [ 'test.set.circleA' ] ]
                ] );
-               mw.loader.using( 'test.circle3' ).then(
+               mw.loader.using( 'test.set.circleC' ).then(
                        function done() {
                                assert.ok( false, 'Unexpected resolution, expected error.' );
                        },
@@ -154,11 +154,11 @@
                mw.redefineFallbacksForTest();
 
                mw.loader.register( [
-                       [ 'test.shim.circle1', '0', [ 'test.shim.circle2' ] ],
-                       [ 'test.shim.circle2', '0', [ 'test.shim.circle3' ] ],
-                       [ 'test.shim.circle3', '0', [ 'test.shim.circle1' ] ]
+                       [ 'test.shim.circleA', '0', [ 'test.shim.circleB' ] ],
+                       [ 'test.shim.circleB', '0', [ 'test.shim.circleC' ] ],
+                       [ 'test.shim.circleC', '0', [ 'test.shim.circleA' ] ]
                ] );
-               mw.loader.using( 'test.shim.circle3' ).then(
+               mw.loader.using( 'test.shim.circleC' ).then(
                        function done() {
                                assert.ok( false, 'Unexpected resolution, expected error.' );
                        },
@@ -172,9 +172,9 @@
        QUnit.test( '.load() - Error: Circular dependency', function ( assert ) {
                var capture = [];
                mw.loader.register( [
-                       [ 'test.circleA', '0', [ 'test.circleB' ] ],
-                       [ 'test.circleB', '0', [ 'test.circleC' ] ],
-                       [ 'test.circleC', '0', [ 'test.circleA' ] ]
+                       [ 'test.load.circleA', '0', [ 'test.load.circleB' ] ],
+                       [ 'test.load.circleB', '0', [ 'test.load.circleC' ] ],
+                       [ 'test.load.circleC', '0', [ 'test.load.circleA' ] ]
                ] );
                this.sandbox.stub( mw, 'track', function ( topic, data ) {
                        capture.push( {
@@ -184,11 +184,11 @@
                        } );
                } );
 
-               mw.loader.load( 'test.circleC' );
+               mw.loader.load( 'test.load.circleC' );
                assert.deepEqual(
                        [ {
                                topic: 'resourceloader.exception',
-                               error: 'Circular reference detected: test.circleB -> test.circleC',
+                               error: 'Circular reference detected: test.load.circleB -> test.load.circleC',
                                source: 'resolve'
                        } ],
                        capture,
@@ -196,6 +196,31 @@
                );
        } );
 
+       QUnit.test( '.load() - Error: Circular dependency (direct)', function ( assert ) {
+               var capture = [];
+               mw.loader.register( [
+                       [ 'test.load.circleDirect', '0', [ 'test.load.circleDirect' ] ]
+               ] );
+               this.sandbox.stub( mw, 'track', function ( topic, data ) {
+                       capture.push( {
+                               topic: topic,
+                               error: data.exception && data.exception.message,
+                               source: data.source
+                       } );
+               } );
+
+               mw.loader.load( 'test.load.circleDirect' );
+               assert.deepEqual(
+                       [ {
+                               topic: 'resourceloader.exception',
+                               error: 'Circular reference detected: test.load.circleDirect -> test.load.circleDirect',
+                               source: 'resolve'
+                       } ],
+                       capture,
+                       'Detect a direct self-dependency'
+               );
+       } );
+
        QUnit.test( '.using() - Error: Unregistered', function ( assert ) {
                var done = assert.async();