From: Timo Tijhof <krinklemail@gmail.com>
Date: Thu, 9 Aug 2018 01:27:08 +0000 (+0100)
Subject: resourceloader: Move add() outside loop to optimise sortDependencies()
X-Git-Tag: 1.34.0-rc.0~4505^2
X-Git-Url: https://git.cyclocoop.org/%27.WWW_URL.%27admin/?a=commitdiff_plain;h=d60d5d7a4728e984a4f82773977c4e9df9f7fc74;p=lhc%2Fweb%2Fwiklou.git

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 a2ba85fb7a..63d23d8d3a 100644
--- 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 6b2738b2e7..05b85be3a4 100644
--- 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();