Wikimedia Bugzilla is closed!

How could we possibly detect this case? The script tag is produced server-side.

Also, what kind of extension module would load 'site' or 'user'? That sounds evil to me.

On zhwiki, there were a bunch of JavaScript tools depending on wgULS which is a converter for zh variants and was defined in Common.js and those tools worked fine. After some time they don't work anymore because wgULS is not defined. I tried to add mw.loader.using('site', to them and found the site JS run twice and filed this bug.

To fix the problem you had on zhwiki, I suggest putting ULS in a hidden gadget (are hidden gadgets supported on the live site? I forgeT) so you can use mw.loader.using( 'ext.gadgets.uls', ... ) as appropriate.

Re the actual bug, maybe we should disallow calling load() on site and user somehow. They're weird modules because they're loaded raw, which means the usual guards against double-loading fail; however, they're also loaded on every page, so calling load() on them is always unnecessary. So there the good news is your load() call would stop working (which fixes the double-loading bug), and the bad news is your load() call would stop working (which breaks your hack).

(In reply to comment #3)
> To fix the problem you had on zhwiki, I suggest putting ULS in a hidden gadget
> (are hidden gadgets supported on the live site? I forgeT) so you can use
> mw.loader.using( 'ext.gadgets.uls', ... ) as appropriate.

That's exactly what I did at http://zh.wikipedia.org/wiki/MediaWiki:Gadget-site-lib.js . I set the description text to a blank string and hid the checkbox with site CSS. I cannot find an option to set a gadget as hidden, even in trunk.

> Re the actual bug, maybe we should disallow calling load() on site and user
> somehow. They're weird modules because they're loaded raw, which means the
> usual guards against double-loading fail; however, they're also loaded on every
> page, so calling load() on them is always unnecessary. So there the good news
> is your load() call would stop working (which fixes the double-loading bug),
> and the bad news is your load() call would stop working (which breaks your
> hack).

Why do we have to load them raw? or can we wrap the whole content with an if(){} of loading state check.

(In reply to comment #4)
> Why do we have to load them raw? or can we wrap the whole content with an
> if(){} of loading state check.
We didn't want to load them in a closure like normal modules, because that could break function definitions that are expected to be global etc. It should be OK to wrap them in an if statement or do anything else that doesn't involve wrapping them in a closure, though.

Ran into this problem at the Commons now.

Site module is loaded by default and does a mw.loader.state('site', 'ready') at the end. However, a gadget used mw.loader.using(['site, 'user'], ...) and got executed earlier. mw.loader.using didn't know about 'site' being in transit, and issued a second request. (Ditto for 'user'.) That second request even arrived and executed before the default request. Because it ran the site module in a closure, we indeed hit an "undefined variable exception" later on in the gadget.

This particular error has been fixed by replacing a "var Foo = {...}" in the Commons' Common.js by "window.Foo = {...}", but still loading this module twice may have arbitrary side-effects.

mw.loader.using really should special-case the default loaded modules "site", "user", "user.groups". It is evidently useful to be able to specify them as dependencies in mw.loader.using(), but we don't want to load them a second time.

So why not simply adapt mw.loader to have the default-loaded modules pre-registered with a 'loading' state?

As a temporary work-around for the site module, I think wrapping the whole Common.js into

if( mw.loader.getState( 'site' ) !== 'ready' ) {
...
}

should work. Roan, is this correct?

This work-around won't work for the user module, though. mw.loader.using requests the user module without the "&user=" parameter and thus gets back an empty module. If that arrives first, mw.loader may consider the dependency to be fulfilled although the real user module hasn't been loaded/executed yet.

Now that is a bug, and therefore I'm elevating this from "enhancement" to "normal".

(In reply to comment #1)

> Also, what kind of extension module would load 'site' or 'user'? That sounds
> evil to me.

Not evil at all. Use cases from the Commons:

Common.js implements some cookie-based preference mechanism called JSconfig for gadgets and other scripts. 

(The code for this is in Common.js because that used to be loaded also on Special:Preferences, so the script could add UI elements allowing the user to comfortably set these preferences. I just noticed that Common.js isn't loaded anymore on Special:Preferences, so that doesn't work anymore. However, I notice that the "user.groups" module is loaded on Special:Preferences...)

Gadgets who want to query these JSconfig settings have a dependency on the "site" module.

Another way to define settings for gadgets that is widely employed is by letting users configure gadget behavior through flags and variables set in their user JS. That, however, means that gadgets using this mechanism have a dependency on the "user" module.

Both mechanisms used to work fine, because user JS and site scripts were loaded before gadgets executed. Sine MW 1.19 (or maybe already 1.18), that is no longer true.

So, in summary, we need:
* A way to set gadget preferences. Maybe by having some JS loaded and executed also on Special:Preferences. Or give us a new page Special:GadgetPreferences where JS can run and add prefs. Or some way to specify declaratively which preferences a certain gadget has.
* A way of specifying dependencies on the default-loaded modules without causing them to be loaded multiple times.

Created attachment 10367 [details]
Patch implementing the proposed solution: pre-register default modules with state 'loading'

This patch does prevent the double loading of the default modules if they're used as explicit dependencies (or if someone tries to load them explicitly).

Please note the FIXME in the patch; it appears to me that this is an issue that exists in the current code and which is not fixed by this patch.

Created attachment 10368 [details]
Patch implementing the proposed solution: pre-register default modules with state 'loading'

Fixes a typo.

Lupo, could you put this patch in Gerrit to make it easier to review?

https://www.mediawiki.org/wiki/Git/Workflow#How_to_submit_a_patch

Thanks!

https://gerrit.wikimedia.org/r/4326

Rm need-review as it's in Git now.

Follow-up change https://gerrit.wikimedia.org/r/5929 (whitespace and a typo in a comment).

See also bug 35240: if a wiki has site or user modules disabled altogether, they'll now end up with state "missing" in the client-side loader. However, the state machine in mw.loader is incomplete and doesn't handle failing modules correctly.

*** Bug 29359 has been marked as a duplicate of this bug. ***