From 0f69149f798345dbe69be8540ba2c80aa7b2a8b6 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Sat, 29 Aug 2015 00:56:24 +0200 Subject: [PATCH] resourceloader: Consistently set state=ready after script execution (not before) For the case of 'module.script' being an array, mw.loader already sets state=ready after execution. For the cases of it being a function or string, we were setting state=ready before execution. This looks like it may have intended to prevent double execution where e.g. some other module may need this module and see it has state 'loading' and start executing it. However ResourceLoader doesn't expose execute(). The only code calling execute() is code that also sets 'state=loading', or handlePending. In fact, this early setting of "ready" could actually cause double execution. Not of the current module, but of dependent modules. If a module's script loads other modules that are cache hits, that could may trigger handlePending() which will assume the current module to be ready, when it isn't. Now that these code paths match, re-use the markModuleReady callback instead of repeating this code. Update tests: * Make assertion captions less verbose. * Change load instruction from "test.implement" to "test.implement.import". Module "test.implement" doesn't exist. This worked by accident because implement() can sometimes run a module immediately if it doesn't exist. * Update test to reflect internal detail that state=loading during script execution. * Assert afterwards that script ran and state=ready. Change-Id: I6b0542edb113d58b8f24cc8587e98ee88b514c55 --- resources/src/mediawiki/mediawiki.js | 9 +++------ .../suites/resources/mediawiki/mediawiki.test.js | 14 ++++++++------ 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/resources/src/mediawiki/mediawiki.js b/resources/src/mediawiki/mediawiki.js index 56a4699b6a..2632902a25 100644 --- a/resources/src/mediawiki/mediawiki.js +++ b/resources/src/mediawiki/mediawiki.js @@ -1210,9 +1210,8 @@ } else if ( $.isFunction( script ) ) { // Pass jQuery twice so that the signature of the closure which wraps // the script can bind both '$' and 'jQuery'. - registry[module].state = 'ready'; script( $, $ ); - handlePending( module ); + markModuleReady(); } else if ( typeof script === 'string' ) { // Site and user modules are a legacy scripts that run in the global scope. // This is transported as a string instead of a function to avoid needing @@ -1221,14 +1220,12 @@ // Implicit dependency on the site module. Not real dependency because // it should run after 'site' regardless of whether it succeeds or fails. mw.loader.using( 'site' ).always( function () { - registry[module].state = 'ready'; $.globalEval( script ); - handlePending( module ); + markModuleReady(); } ); } else { - registry[module].state = 'ready'; $.globalEval( script ); - handlePending( module ); + markModuleReady(); } } } ); diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js index c8f00115eb..fcf6dda9f1 100644 --- a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js +++ b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js @@ -630,20 +630,20 @@ } ); // @import (bug 31676) - QUnit.asyncTest( 'mw.loader.implement( styles has @import)', 5, function ( assert ) { + QUnit.asyncTest( 'mw.loader.implement( styles has @import)', 7, function ( assert ) { var isJsExecuted, $element; mw.loader.implement( 'test.implement.import', function () { - assert.strictEqual( isJsExecuted, undefined, 'javascript not executed multiple times' ); + assert.strictEqual( isJsExecuted, undefined, 'script not executed multiple times' ); isJsExecuted = true; - assert.equal( mw.loader.getState( 'test.implement.import' ), 'ready', 'module state is "ready" while implement() is executing javascript' ); + assert.equal( mw.loader.getState( 'test.implement.import' ), 'loading', 'module state during implement() script execution' ); $element = $( '
Foo bar
' ).appendTo( '#qunit-fixture' ); - assert.equal( mw.msg( 'test-foobar' ), 'Hello Foobar, $1!', 'Messages are loaded before javascript execution' ); + assert.equal( mw.msg( 'test-foobar' ), 'Hello Foobar, $1!', 'messages load before script execution' ); assertStyleAsync( assert, $element, 'float', 'right', function () { assert.equal( $element.css( 'text-align' ), 'center', @@ -666,8 +666,10 @@ } ); - mw.loader.load( 'test.implement' ); - + mw.loader.using( 'test.implement.import' ).always( function () { + assert.strictEqual( isJsExecuted, true, 'script executed' ); + assert.equal( mw.loader.getState( 'test.implement.import' ), 'ready', 'module state after script execution' ); + } ); } ); QUnit.asyncTest( 'mw.loader.implement( dependency with styles )', 4, function ( assert ) { -- 2.20.1