Page MenuHomePhabricator

ResourceLoader should restrict addModuleStyles() to modules that only provide styles
Closed, ResolvedPublic

Description

Announcement and current proposal

See Wikitech: https://lists.wikimedia.org/pipermail/wikitech-l/2016-May/085479.html

For migration of site and user module:

Currently:

  • Output A: <link modules=site> (loads site styles), mw.loader.load('site') (loads site scripts, and styles a second time)

    Step 1:
  • Create 'site.styles'. – <https://gerrit.wikimedia.org/r/292972 >
  • Start tracking state of pure style modules. – https://gerrit.wikimedia.org/r/288026
  • Behaviour of Output A unchanged.
  • New Output B: <link modules=site.styles>, state(site.styles = ready), mw.loader.load('site') (still loads styles twice)

    Step 2:
  • Remove styles from 'site', make 'site' depend on 'site.styles' (only affects startup module). – https://gerrit.wikimedia.org/r/292973
  • No change in HTML output.
  • Output B no longer ends up loading styles twice.

Original proposal

We'll need to grandfather in the legacy 'user' and 'site' module that need global scope execution, and load stylesheets separately. For anything else, addModuleStyles should throw (or ignore and log warning) for any module that does not purely have styles.

So it should not allow modules with scripts or dependencies.

@TrevorParscal, @Catrope and I discussed this a few weeks ago and decided to go in this direction. It will also serve to fulfill a request made to the Frontend Standard Group for future flexibilities and performance improvements (Phabricator tasks?) that depend on this.

From what I recall the implementation would add methods to the ResourceLoaderModule base class to indicate whether a module has (only) styles (possibly more generic with ability to detect other purity as well).

Modules default to not allowing addModuleStyles in the new system and require implementing this new method.

For simple modules like ResourceLoaderFileModule we'll provide the method, as it can be determined from the constructor without any computation. For code-generating modules it can not be automatically determined if getStyles/getScripts will provide something or not. Those will need to implement the method if they need addModuleStyles.

As a result of this distinction, we can safely output mw.loader.state( .., "ready" ) for modules loaded via addModuleStyles.

This will fix bugs such as:

This will enable:

Status

  • Implement.
  • Monitor resourceloader logs from Jenkins, Beta and mwdebug requests in production for any "Unexpected general module "{module}" in styles queue." - fix any violations in core and extensions.
  • Release deprecation warning in 1.29
  • Once low enough for Gadgets as well, change Gadgets extension to use "general" (addModules) by default (for modules that contain both scripts and styles). – https://gerrit.wikimedia.org/r/353586
  • (No production and beta warnings in the logs.) Consider changing addModuleStyles() in MediaWiki to no longer load such module, instead ignore and log non-fatal error. (Release in 1.30?) – https://gerrit.wikimedia.org/r/362121

Logstash warnings

https://logstash.wikimedia.org/goto/6a23ca1975a524a20e452b2b57e839c3

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Krinkle updated the task description. (Show Details)May 2 2017, 12:49 AM

Change 351228 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Bump severity of style queue violation to Warning

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

Change 351229 merged by jenkins-bot:
[mediawiki/core@REL1_29] resourceloader: Bump severity of style queue violation to Warning

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

Change 352990 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/CodeReview@master] Fix styles queue violation for "ext.codereview.local"

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

Krinkle updated the task description. (Show Details)May 10 2017, 1:40 AM
Krinkle removed a project: MW-1.29-release-notes.
Krinkle updated the task description. (Show Details)
Krinkle updated the task description. (Show Details)May 10 2017, 1:43 AM

Change 352997 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/TemplateData@master] Fix styles queue violation for "ext.templateData"

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

Krinkle updated the task description. (Show Details)May 10 2017, 1:49 AM

Change 352990 merged by jenkins-bot:
[mediawiki/extensions/CodeReview@master] Fix styles queue violation for "ext.codereview.local"

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

Change 353101 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/CodeReview@wmf/1.29.0-wmf.21] Fix styles queue violation for "ext.codereview.local"

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

Krinkle updated the task description. (Show Details)May 10 2017, 4:47 PM

Change 353101 merged by jenkins-bot:
[mediawiki/extensions/CodeReview@wmf/1.29.0-wmf.21] Fix styles queue violation for "ext.codereview.local"

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

Change 352997 merged by jenkins-bot:
[mediawiki/extensions/TemplateData@master] Fix styles queue violation for "ext.templateData"

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

Change 353147 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/CodeReview@wmf/1.30.0-wmf.1] Fix styles queue violation for "ext.codereview.local"

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

Change 353147 merged by jenkins-bot:
[mediawiki/extensions/CodeReview@wmf/1.30.0-wmf.1] Fix styles queue violation for "ext.codereview.local"

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

Change 353343 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/TemplateData@wmf/1.30.0-wmf.1] Fix styles queue violation for "ext.templateData"

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

Change 353343 merged by jenkins-bot:
[mediawiki/extensions/TemplateData@wmf/1.30.0-wmf.1] Fix styles queue violation for "ext.templateData"

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

Mentioned in SAL (#wikimedia-operations) [2017-05-11T23:30:52Z] <thcipriani@tin> Synchronized php-1.30.0-wmf.1/extensions/TemplateData/extension.json: SWAT: [[gerrit:353343|Fix styles queue violation for "ext.templateData"]] T92459 (duration: 00m 39s)

Change 353586 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/Gadgets@master] Remove duplicate loading of styles (assume type=general if content is mixed)

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

Change 353586 merged by jenkins-bot:
[mediawiki/extensions/Gadgets@master] Remove duplicate loading of styles (assume type=general if content is mixed)

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

Krinkle updated the task description. (Show Details)May 24 2017, 3:40 PM

Change 355439 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/wikihiero@master] Fix styles queue violation for "ext.wikihiero.Special"

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

Change 355441 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/wikihiero@wmf/1.30.0-wmf.2] Fix styles queue violation for "ext.wikihiero.Special"

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

Change 355439 merged by jenkins-bot:
[mediawiki/extensions/wikihiero@master] Fix styles queue violation for "ext.wikihiero.Special"

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

Change 355441 merged by jenkins-bot:
[mediawiki/extensions/wikihiero@wmf/1.30.0-wmf.2] Fix styles queue violation for "ext.wikihiero.Special"

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

Mentioned in SAL (#wikimedia-operations) [2017-05-24T22:33:16Z] <krinkle@tin> Synchronized php-1.30.0-wmf.2/extensions/wikihiero: Fix styles queue warning - T92459 (duration: 00m 42s)

Krinkle updated the task description. (Show Details)May 26 2017, 10:49 AM
Krinkle updated the task description. (Show Details)
Krinkle updated the task description. (Show Details)
Krinkle updated the task description. (Show Details)Jun 7 2017, 5:57 PM

Change 358158 had a related patch set uploaded (by Tpt; owner: Tpt):
[mediawiki/extensions/ProofreadPage@master] Restricts modules loaded by addModuleStyles() to styles only

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

Change 358158 merged by jenkins-bot:
[mediawiki/extensions/ProofreadPage@master] Restricts modules loaded by addModuleStyles() to styles only

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

Change 362121 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Restrict addModuleStyles() to type=styles modules

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

Krinkle updated the task description. (Show Details)Jun 29 2017, 2:18 AM

Change 362260 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/OpenStackManager@master] Split ext.openstack.base JavaScript from ext.openstack style module

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

Krinkle updated the task description. (Show Details)Jun 29 2017, 6:48 PM
Krinkle updated the task description. (Show Details)Jun 29 2017, 6:53 PM
Krinkle moved this task from Not yet to Write about next on the Performance-Team-publish board.

Change 362260 merged by jenkins-bot:
[mediawiki/extensions/OpenStackManager@master] Split ext.openstack.base JavaScript from ext.openstack style module

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

Change 362121 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Restrict addModuleStyles() to type=styles modules

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

Krinkle closed this task as Resolved.Jul 1 2017, 2:03 AM
Krinkle removed a project: Patch-For-Review.
Krinkle updated the task description. (Show Details)