r88053 MediaWiki - Code Review archive

+		 * @param style Object: Object of CSS strings keyed by media-type or Object of lists of URLs
+		 * keyed by media-type
+		 * as the href attribute when adding a link element to the head

The last line looks out of place.

+			if ( !$.isPlainObject( style ) ) {
+				throw new Error( 'style must be a object or a string, not a ' + typeof style );
+			}

"A object" isn't correct English, and strings don't seem to be allowed here.

Trevor fixed this in r88074.

+		function handlePending() {

 					if ( $.inArray( module, jobs[j].dependencies ) !== -1 ) {

module is not defined here. Causes ReferenceError: Can't find variable: module when (for example) tablesorter is loaded after dom-ready (try on a page with a <table class="sortable">).

I guess passing module should do it although I'm not quite sure, this function is a little magic to me.

Doesn't err in production mode (either not happening or silently ignored, I guess the latter)

Fixed in r88329.

+				if ( $.isFunction( callback ) ) {
+					// Document.write is synchronous, so this is called when it's done
+					callback();
+				}

Lies! document.write() is not synchronous, at least not in Firefox 4.0.1, but I understand why it would appear to be. The document.write() is deferred: it's queued and the script tag is only written to the DOM after this script's thread finishes, i.e. some time after the callback has run to completion. However, in your use case (debug mode where all modules are file-based) the only thing in the callback was more calls to document.write() (indirectly, via addScript()). This means that if B depends on A, the document.write() for B is executed before A is loaded. That's not a problem, though, because the document.write() calls are serialized properly, so the script tags are written to the DOM in the right order, and executed sequentially.

This house of cards collapses when you're mixing file-based and wiki page-based modules in debug mode, though, or generally when mixing modules of the form mw.loader.implement(["some url"], {}, {}); (which is how file modules work in debug mode) with modules of the form mw.loader.implement(function($) { code; }, {}, {}); (which is how wiki page modules work in both debug and production mode). If a wiki page module depends on a file module, the document.write() call for the file module will be executed before the wiki page module is executed, but because the document.write() is deferred the wiki page module ends up being executed before its dependency. This is happening in the WikiLove extension with ext.wikiLove.local (which is a wiki page module and depends on ext.wikiLove.startup, which is a file-based module). I investigated Jan Paul's complaints and found this.

I'm not really sure how this should be fixed. At the bottom of the body, document ready hasn't happened yet but the document is ready enough to do async script loading as done in the if ( ready ) { branch (at least in FF4). At the top of the body, this doesn't work, but this can be detected with if ( document.body ) { (at least in FF4). However, doing that (I tried it locally) introduces another set of problems. Because load() is no longer synchronous before document ready, modules like mw.util and mw.user can't just be assumed to be present. This means dependencies for mw.util have to be flagged explicitly and the inlined mw.user.options and mw.user.tokens modules have to be wrapped in mw.loader.using( 'mw.user', function() { ... } ); . Worse still, it means site and user scripts will typically execute before mw.util and mw.user or any of the other stuff that's loaded at the bottom of the body.

I've tried wrapping the callback(); invocation in setTimeout( ..., 0 ); but that fails catastrophically because code run through setTimeout() executes in a different thread where, apparently, jQuery isn't available and IFFEs fail.

Other solutions that could be explored are:

• return the code as a string instead of a closure, and inject that into the DOM as a script tag using document.write() . We basically do the same thing for CSS already. A major disadvantage is that we won't get proper line numbers and that tracking down the origin of a piece of code is non-trivial. These are bad things in debug mode
• URLify non-file modules too, using only=scripts. This seems like the most promising way to go forward to me

+					for ( var i = 0; i < script.length; i++ ) {
+						registry[module].state = 'loading';
+						addScript( script[i], function() {
+							if ( ++done == script.length ) {
+								registry[module].state = 'ready';
+								handlePending();
+								if ( $.isFunction( callback ) ) {
+									callback();
+								}
+							}
+						} );
+					}

This code also has a bug, which I found when experimenting with loading things asynchronously. When loading a module that consists of multiple scripts (in my case, this was ext.wikiLove.startup), this code issues a separate addScript() for each file, then increment a shared variable done so the callback is only called when all files are loaded. However, this potentially breaks loading order guarantees for files within the module: when doing addScript(A); addScript(B); , A and B are requested in parallel and executed when they arrive, which means it's perfectly possible for B to execute before A. This violates assumptions that module writers make about multi-file modules always being loaded in the specified order (this happens in production mode), and broke the WikiLove module I mentioned because the first file defined $.wikiLove = { ... }; and the second file extended it using $.wikiLove.foo = ...; .

The bug I mentioned at the very end of the comment (about breaking loading order within the module), is fixed in r96964.

When this is fixed, could you check that Bug #29606 is also fixed?

Hm.. I'm not sure I see the link here ?

Sorry, I meant Bug #29608

bug 22094 ;-)

There may be hope for document.write:

https://developer.mozilla.org/En/HTML/Element/Script#Gecko-specific_notes

Roan wrote:

URLify non-file modules too, using only=scripts. This seems like the most promising way to go forward to me

Seems like the logical choise to me, that avoids the work-around crap all together.

btw, all of the above only applies to debug mode. Production mode is OK.

I implemented this in r96978. Setting to resolved, because all of my concerns with this revision have been addressed.

When loading a module that has stylesheets for non-screen media (such as 'skins.modern' which has a 'media' => 'print' stylesheet), this revision loads the print stylesheet and the screen regardless and the browsers also gets to execute them.

This usually only happens in debug mode, caused by this revision:

 				for ( var media in registry[module].style ) {
-					$marker.before( mw.html.element( 'style',
-						{ type: 'text/css', media: media },
-						new mw.html.Cdata( registry[module].style[media] )
-					) );
+					var style = registry[module].style[media];
+					if ( $.isArray( style ) ) {
+						for ( var i = 0; i < style.length; i++ ) {
+							$marker.before( mw.html.element( 'link', {
+								'type': 'text/css',
+								'rel': 'stylesheet',
+								'href': style[i]
+							} ) );
+						}
+					} else if ( typeof style === 'string' ) {
+						$marker.before( mw.html.element(
+							'style',
+							{ 'type': 'text/css', 'media': media },
+							new mw.html.Cdata( style )
+						) );
+					}
 				}

Previously it used the media property. In the new code it only uses it for ‎<style> but not in the ‎<link>.