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:

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

Details

SubjectRepoBranchLines +/-
mediawiki/coremaster+6 -6
mediawiki/extensions/OpenStackManagermaster+6 -5
mediawiki/extensions/ProofreadPagemaster+22 -28
mediawiki/extensions/wikihierowmf/1.30.0-wmf.2+13 -13
mediawiki/extensions/wikihieromaster+13 -13
mediawiki/extensions/Gadgetsmaster+7 -27
mediawiki/extensions/TemplateDatawmf/1.30.0-wmf.1+2 -4
mediawiki/extensions/TemplateDatamaster+2 -4
mediawiki/extensions/CodeReviewwmf/1.30.0-wmf.1+5 -22
mediawiki/extensions/CodeReviewwmf/1.29.0-wmf.21+5 -22
mediawiki/extensions/CodeReviewmaster+5 -22
mediawiki/coreREL1_29+3 -1
mediawiki/coremaster+3 -1
mediawiki/extensions/Gadgetsmaster+143 -5
mediawiki/corewmf/1.28.0-wmf.13+102 -26
mediawiki/coremaster+102 -26
mediawiki/coremaster+7 -6
mediawiki/coremaster+77 -0
mediawiki/coremaster+49 -1
mediawiki/coremaster+6 -52
Show related patches Customize query in gerrit

Related Objects

StatusSubtypeAssignedTask
ResolvedKrinkle
ResolvedKrinkle
ResolvedTheDJ
ResolvedTheDJ
ResolvedKrinkle
ResolvedKrinkle
Resolved Mattflaschen-WMF
ResolvedFlorian
Resolved Mattflaschen-WMF
Resolved Mattflaschen-WMF
Resolved Mattflaschen-WMF
ResolvedPRODUCTION ERRORPhysikerwelt
ResolvedPRODUCTION ERRORJdlrobson
ResolvedPRODUCTION ERRORAddshore
Resolvedmatmarex
ResolvedPRODUCTION ERRORTpt

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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 removed a project: MW-1.29-release-notes.
Krinkle updated the task description. (Show Details)

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

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

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

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)
Krinkle updated the task description. (Show Details)

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

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

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 removed a project: Patch-For-Review.
Krinkle updated the task description. (Show Details)