Last modified: 2014-11-17 10:34:44 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T29023, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 27023 - After the document is ready mw.loader is broken (calls callback before module is parsed)
After the document is ready mw.loader is broken (calls callback before module...
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
ResourceLoader (Other open bugs)
unspecified
All All
: Low critical with 1 vote (vote)
: ---
Assigned To: Krinkle
:
Depends on:
Blocks: 25962 28151
  Show dependency treegraph
 
Reported: 2011-01-29 00:14 UTC by Michael Dale
Modified: 2014-11-17 10:34 UTC (History)
9 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
mediawiki pre-patch for loading issue (4.13 KB, patch)
2011-01-29 00:16 UTC, Michael Dale
Details

Description Michael Dale 2011-01-29 00:14:49 UTC
Once the document is ready the resource loader does script loading asyncronusyly. The implement call seems to only work in syncronus mode. 

In preparing for a patch ran into the following: 

The is document ready flow is repeated twice it should be a local function. 

Additionally their is shadow overcast scope of the "ready" variable with the .using function can be fixed by renaming.

patch attached.
Comment 1 Michael Dale 2011-01-29 00:16:07 UTC
Created attachment 8062 [details]
mediawiki pre-patch for loading issue

fixes function shadow of 'ready' variable, abstracts the document ready check for script appending
Comment 2 Michael Dale 2011-01-31 22:46:36 UTC
I am bumping this to critical ( with hopes that it becomes blocking )... I believe Trevor is working on it now. Its documented as a supported use case of the resource loader that is broken. If not going to be blocking for a targeted release, then the resource loader should at least throw an exception or error if you try to load anything after the DOM is ready rather than issue the 'ready' callback.
Comment 3 Rob Lanphier 2011-02-02 21:18:38 UTC
Marking as blocker to bug 26611 for discussion
Comment 4 Derk-Jan Hartman 2011-02-06 18:24:58 UTC
@mdale, is document.write() safe to use in that location ? As far as I know, you shouldn't never use it other than during inline script execution...
Comment 5 Michael Dale 2011-02-06 19:24:10 UTC
Setting loader.using to be only pre-dom ready would be bad. The mediaWiki code documentation that says the callback should be issued once the modules are 'ready' and the code includes stubs for checking if it can do a document.write vs a script append. As mentioned above if not supported it should fire an error. But feature wise its something that /should/ be supported, else people will have to wrap the resource loader functionality with ugly hacks anytime they want to load after DOM is ready. If random client code becomes responsible manually loading decencies post DOM ready it will get messy and difficult to avoid re-loading the same module twice. 

There are lots of reasons you may want to load dependencies after the DOM is ready. Like a gadget that searches the page for a particular dom id or css class or config, or sub-components invoked on user interaction like an add-media-wizard pulling in an 'upload wizard' interface when you click the 'upload' button on edit pages ( without loading all of this every time you visit the base edit page ) or anytime you have asynchronous build out of components with dynamic dependencies ( ie checking the API or DOM some information that affects the set of interfaces that you load for that page ...
or simply for performance reasons you may want to display the page before every possible user interface and sub component is loaded and blinded.
Comment 6 Mark A. Hershberger 2011-02-10 17:41:09 UTC
Any news on this since it is blocking the tarball?
Comment 7 Michael Dale 2011-02-24 05:37:53 UTC
I have sent Trevor some more info about the bug, how to recreate it etc.
Comment 8 Lupo 2011-02-25 07:58:18 UTC
Hm, Michael, I thought script.onload didn't work on IE? I thought you needed to
use script.onreadystatechange on IE. See also the implementation of jQuery.ajax (line 5075).

Also, could you explain what the $.globalEval issue is that makes you avoid using jQuery.getScript in the first place?
Comment 9 Michael Dale 2011-02-25 15:40:16 UTC
Lupo your right. Older versions of IE need  onreadystatechange furthermore IE sometimes fire the onreadystatechange event before the script has been fully parsed. So you get symbol undefined errors when you directly run the callback.

jQuery GlobalEval address this issue, but makes debugging more tricky, since you don't always get line numbers for syntax errors. 

These issues point to either have per-browser debug mode hacks, or going back to loading resources through the resource loader in debug mode ( so that it can append a ready callback call. That way you can use script append ( without globalEval ) and you can get line numbers for errors. 

In the old resource loader I had every class you load named by what it defines, so it would check that the target variable was defined before issuing the callback, ( letting me use script append and not worrying about onload / globalEval issues )... But that of course won't work here, and in general applies too much constraint to resource definitions.
Comment 10 Michael Dale 2011-03-11 20:13:42 UTC
So this has partially be solved in trunk by the use of $('body').append( script ); call which issues a synchronous / blocking xhr request and evals the result. Causing the mediawiki.loader.using to block on the loading of scripts and issue the callback at the right time. 

This of course is not fun because it blocks all other script execution while the module set is being loaded ( both in debug and non-debug modes ) and when in debug mode is makes it nearly impossible to "debug" since it evals all the scripts that it appends to the document giving syntax errors in the parent script include line instead of the script error line. Trevor and I are scheduled to review a solution to these issues on Monday.
Comment 11 Michael Dale 2011-03-21 18:03:04 UTC
Trevor, I had an idea for improved debug mode post dom ready without timeout hacks waiting for parsing and without synchronous XHR eval script execution. 

We should include all the scripts by normal document append and instead of waiting for the inconsistently fired "onload" "onreadystatechange" without the files being full parsed, We do normal script appends and after each script we append a separate resource loader "callback" script include. With IE we set the defer attribute to true for the special "callback" script and all the other ~sane~ browsers will execute in the order appended to the dom and issue the onload callback once the script is actually parsed. 

This way we retain line numbers for syntax errors and direct reference to raw resources without doing too much of a hack for IE.
Comment 12 Trevor Parscal 2011-03-22 15:13:04 UTC
Sounds clever. We need to get together to make this happen.
Comment 13 Mark A. Hershberger 2011-03-28 19:50:35 UTC
We deployed with this bug, so I don't think it should be a tarball blocker.
Comment 14 Mark A. Hershberger 2011-04-02 03:34:00 UTC
Lowering priority so I don't bother with this any more until you two get a chance to fix it.
Comment 15 p858snake 2011-04-30 00:09:19 UTC
*Bulk BZ Change: +Patch to open bugs with patches attached that are missing the keyword*
Comment 16 Michael Dale 2011-05-13 15:25:03 UTC
the globalEval issues discussed in this thread were resolved with a something similar to the patch in this head of this bug applied in r87986
Comment 17 Krinkle 2011-05-17 17:27:29 UTC
QUnit test suite now contains a test specifically for this. 

Link to test:

http://localhost/mediawiki/trunk/phase3/resources/test/?filter=mediawiki.js%3A%20mw.loader

Please re-open if still failing, preferably with a test case to reproduce this in a neutral environment (ie. QUnit)
Comment 18 Michael Dale 2011-05-17 21:41:55 UTC
Yes this should be fixed now, the test case looks good.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links