Page MenuHomePhabricator

Loading with addModuleStyles() should set module state to "ready"
Closed, ResolvedPublic

Description

Using addModuleStyles() and then addModules() for one module on one page duplicates the styles. Same with addModuleScripts(), but that doesn't come up often in practice.

That is a feature, not a bug, but we're planning to kill it so let's put this here. (Also came up on https://gerrit.wikimedia.org/r/#/c/182875/.)

Related Objects

Event Timeline

matmarex created this task.Jan 29 2015, 6:30 PM
matmarex updated the task description. (Show Details)
matmarex raised the priority of this task from to Needs Triage.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 29 2015, 6:30 PM
Aklapper triaged this task as Low priority.Feb 2 2015, 9:46 PM
Krinkle set Security to None.
Jdforrester-WMF removed TrevorParscal as the assignee of this task.Mar 19 2015, 11:32 PM
Florian added a subscriber: Florian.May 2 2015, 4:30 PM
Jdlrobson added a subscriber: Jdlrobson.

We really need to get this fixed.. what is the current thought process around this?

Krinkle renamed this task from Using addModuleStyles() and then addModules() for one module on one page duplicates the styles to Loading with addModuleStyles() should set module state to "ready".Jul 3 2015, 12:17 AM
Krinkle moved this task from Backlog to Accepted: Enhancement on the MediaWiki-ResourceLoader board.
Krinkle added a subscriber: He7d3r.

We really need to get this fixed.. what is the current thought process around this?

Who is "we"? This task is blocked by T92459: ResourceLoader should restrict addModuleStyles() to modules that only provide styles, assigned to Krinkle.

The accepted solution to this issue is to implement a restriction which will then naturally de-duplicate these through the state machine.

However cases where this doesn't result in an (intended) exception and a working page are rare. Most likely the scenario Mobile is in where this is a bug is a situation that will not be supported and result in an exception. So this isn't going to change that. Let's figure out what modules are double loaded here so that we can resolve that instead.

Change 267794 had a related patch set uploaded (by Bartosz Dziewoński):
Work around T87871 to avoid double-loading OOjs UI PHP styles

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

Change 267794 merged by jenkins-bot:
Work around T87871 to avoid double-loading OOjs UI PHP styles

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

Change 288026 had a related patch set uploaded (by Krinkle):
[WIP] resourceloader: Implement getType() and emit warning from addModuleStyles()

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

Krinkle raised the priority of this task from Low to Normal.
Krinkle claimed this task.
Gilles added a subscriber: Gilles.Jun 8 2016, 1:05 PM

Change 288026 merged by jenkins-bot:
resourceloader: Track state of page-style modules

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

If this works, we can remove the workaround for OOjs UI style modules from OutputPage::enableOOUI() and resources/src/oojs-ui-styles-skip.js.

Krinkle added a comment.EditedJul 14 2016, 10:03 PM

@matmarex Soon. There's some outstanding technical debt that's preventing that right now.

OutputPage::headElement() currently forces a needless separation between scripts and styles (coming from different methods, getInlineHeadScripts and buildCssLinks). For performance reasons (09537e83e) we want the scripts before the styles. However, while the "state" initialisation here is extracted from the styles, it is itself a script. As such, this is set *after* the mw.loader.load() call from the main script output. Which means if the dependant module is loaded directly onto a page (e.g. not loaded dynamically via mw.loader at run time), it will still think it isn't loaded and make a separate fetch for it.

This all stems from the hairiness of makeResourceLoaderLink(). Because we deal with each module load individually, we can't re-order this in a meaningful way.

I'm working on a patch that will refactor this so that OutputPage just communicates to a generic queue handler what it wants and let it deal with the HTML output - instead of demanding HTML output for each (array of) modules separately. That way it would output the code in the natural order, which is:

  • Push RLQ script with: mw.config, mw.loader.state(), mw.loader.implement(), mw.loader.load() - in that order.
  • Start async fetch for startup module.
  • Stylesheets.

This would be generated as one chunk, and included by OutputPage::headElement().


Draft at https://gerrit.wikimedia.org/r/299272

Change 299272 had a related patch set uploaded (by Krinkle):
[WIP] resourceloader: Move queue formatting out of OutputPage (DRAFT)

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

Krinkle moved this task from Inbox to Doing on the Performance-Team board.

Change 299272 merged by jenkins-bot:
resourceloader: Move queue formatting out of OutputPage

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

The patch breaks $skin->getOutput()->addModuleStyles(); if used in SkinTemplateNavigation::SpecialPage hook (e.g. Special:Translate).

Change 304787 had a related patch set uploaded (by Gilles):
Move bottomScripts() call in SkinTemplate

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

@Nikerabbit SkinTemplateNavigation::SpecialPage issue should be addressed by this follow-up

Change 304787 merged by jenkins-bot:
Move bottomScripts() call in SkinTemplate

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

Change 305127 had a related patch set uploaded (by Krinkle):
SkinTemplate: Move bottomScripts() back sightly

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

Change 305127 merged by jenkins-bot:
SkinTemplate: Move bottomScripts() back sightly

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

Change 305154 had a related patch set uploaded (by Krinkle):
SkinTemplate: Move bottomScripts() back sightly

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

Change 305154 merged by jenkins-bot:
SkinTemplate: Move bottomScripts() back sightly

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

Krinkle closed this task as Resolved.Aug 17 2016, 4:20 AM

Change 305308 had a related patch set uploaded (by Bartosz Dziewoński):
Revert "Work around T87871 to avoid double-loading OOjs UI PHP styles"

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

Change 305559 had a related patch set uploaded (by Bartosz Dziewoński):
ResourceLoaderImageModule: Mark as style-only

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

Change 305559 merged by jenkins-bot:
ResourceLoaderImageModule: Mark as style-only

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

Change 305308 merged by jenkins-bot:
Revert "Work around T87871 to avoid double-loading OOjs UI PHP styles"

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