User "Catrope" changed the status of MediaWiki.r88053.

Old Status: resolved
New Status: fixme

User "Catrope" also posted a comment on MediaWiki.r88053.

Full URL: 
https://secure.wikimedia.org/wikipedia/mediawiki/wiki/Special:Code/MediaWiki/88053#c17169
Commit summary:

Added direct file loading functionality to debug mode for both scripts and 
styles, with callbacks for module state changes (changing to ready) and 
executing of jobs and modules awaiting dependency resolutions. These changes 
also provide a way to used mw.loader.implement with arrays of URLs for the 
scripts and styles arguments, which will make it possible to implement modules 
using user scripts. This probably solves bug #27023 - tests to verify that will 
be coming soon.

Comment:

<pre>
+                               if ( $.isFunction( callback ) ) {
+                                       // Document.write is synchronous, so 
this is called when it's done
+                                       callback();
+                               }
</pre>
Lies! <code>document.write()</code> is not synchronous, at least not in Firefox 
4.0.1, but I understand why it would appear to be. The 
<code>document.write()</code> 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 <code>document.write()</code> (indirectly, via <code>addScript()</code>). 
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 
<code>document.write()</code> 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 
<code>mw.loader.implement(["some url"], {}, {});</code> (which is how file 
modules work in debug mode) with modules of the form 
<code>mw.loader.implement(function($) { code; }, {}, {});</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 <code>document.write()</code> call for the 
file module will be executed before the wiki page module is executed, but 
because the <code>document.write()</code> 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 <code>if ( ready ) {</code> branch (at least in 
FF4). At the top of the body, this doesn't work, but this can be detected with 
<code>if ( document.body ) {</code> (at least in FF4). However, doing that (I 
tried it locally) introduces another set of problems. Because 
<code>load()</code> 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 
<code>mw.loader.using( 'mw.user', function() { ... } );</code> . 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 <code>callback();</code> invocation in 
<code>setTimeout( ..., 0 );</code> but that fails catastrophically because code 
run through <code>setTimeout()</code> 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 <code>document.write()</code> . 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 <code>only=scripts</code>. This seems like 
the most promising way to go forward to me

<pre>
+                                       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();
+                                                               }
+                                                       }
+                                               } );
+                                       }
</pre>
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 
<code>addScript()</code> for each file, then increment a shared variable 
<code>done</code> 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 <code>addScript(A); addScript(B);</code> , 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 <code>$.wikiLove = { ... };</code> and 
the second file extended it using <code>$.wikiLove.foo = ...;</code> .

_______________________________________________
MediaWiki-CodeReview mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview

Reply via email to