Page MenuHomePhabricator

Load bottom script "site" and "user" asynchronously
Closed, ResolvedPublic

Description

The bottom scripts "site" and "user" currently use a very ugly synchronous script tag inside document.write.

Backstory: (Note that not all arguments are still relevant today, this is just for context.)

  • They're not loaded with mw.loader.load() because these are legacy scripts that needs execution in global scope (or T65728: ResourceLoader: User scripts should not execute in global scope).
  • We generally avoid ajax because of cross-domain in old browsers (though not a concern anymore for WMF since the demise of bits.wikimedia.org), because using XHR means we have to use eval, which means we lose useful stack traces.
  • They're not a plain <script> tag because the request has to be conditional on window.mw - e.g. conditional on the browser being supported by MediaWiki and jQuery, and the startup request having succeeded without network or runtime problems.
  • They're not a plain <script> and don't use <script async> because it needs a callback to update the mw.loader state machine and allow dependant modules to resolve.
  • Unlike regular modules, we can't append a "state()" call to the server response because it's an unwrapped legacy script that may return early or throw exceptions, rendering the "state" call unreachable and causing the loader to hang indefinitely.

But.. what we can do is simply use mw.loader's addScript() which we already use for normal module requests. It creates a script tag in JavaScript (which are always asynchronous) and has a callback.

Event Timeline

Krinkle created this task.Jun 10 2015, 10:27 PM
Krinkle claimed this task.
Krinkle raised the priority of this task from to Normal.
Krinkle updated the task description. (Show Details)
Krinkle added subscribers: Krinkle, Catrope, Gilles.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 10 2015, 10:27 PM
Krinkle moved this task from Tag to Next-up on the Performance board.
Krinkle set Security to None.

Change 221739 had a related patch set uploaded (by Krinkle):
resourceloader: Load bottom 'site' and 'user' scripts asynchronously

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

He7d3r added a subscriber: He7d3r.Jul 9 2015, 4:31 PM
He7d3r updated the task description. (Show Details)Jul 9 2015, 4:35 PM

Change 221739 merged by jenkins-bot:
resourceloader: Implement support for 'site' into mw.loader

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

Change 227912 had a related patch set uploaded (by Krinkle):
[WIP] resourceloader: Ensure 'user' loads after 'site' (asynchronously)

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

Change 227912 merged by jenkins-bot:
resourceloader: Ensure 'user' loads after 'site' (asynchronously)

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

Edokter added a subscriber: Edokter.EditedJul 30 2015, 7:56 PM

Now site and user are loaded twice. @Krinkle calls this 'harmless', but it is extremely annoying having to dig through duplicate properties, not knowing where they come from. I also experience occasional load order issues and discrepancies between bedug and live loads.

Is this double-loading absolutely necessary, or is it a by-product of something else? If the latter, how hard is it to remove this redundant loading?

Now site and user styles are loaded twice. @Krinkle calls this 'harmless', but it is extremely annoying having to dig through duplicate properties, not knowing where they come from. I also experience occasional load order issues and discrepancies between debug and live loads.
Is this double-loading absolutely necessary, or is it a by-product of something else? If the latter, how hard is it to remove this redundant loading?

It is not done on purpose and is the same as T42284 (for Gadgets). The proper solution (which will be enforced by T92459 soon) is to not have a single module used as both addModules() and addModuleStyles().

A module is either a stylesheet module essential at load time, or it is a dynamic module with js and css that is loaded at run time. Modules trying to be both at the same time are not supported.

This is already enforced by code-review and convention in core and extensions. The only exception right now (for legacy reasons) is gadgets and user/site scripts. Until now, the site worked around this by loading the scripts in a dedicated HTTP request (one that can't be cached due to not having a version number in the url, and having to be embedded in the page html). This is now solved and significantly improves caching and run-time performance for end-users like you and me – at the cost of loading the styles twice (which is cancelled out by the gains the first time, and the second page visit it's entirely absent because it's coming from local cache).

In due time we'll either deprecate 'site' in favour of an enabled-hidden gadget (with automatic conversion for existing ones) – or, we'll have to split 'site' into 'site.styles' and 'site.scripts' – or something else we come up with.