Page MenuHomePhabricator

Let ResourceLoader only register skin modules for the current skin
Open, MediumPublic

Description

Motivation

Allow skins to comfortably have more modules at a lower cost.

Background

Right now modules bundles from any installed skin are available for queuing on a page, depending on, or lazy-loading. This is primarily because skins are treated similarly as extensions.

There is technically nothing stopping a skin from registering a generic MW hook and do stuff regardless of which skin is currently used by the user. E.g. logic from any random extension could be merged into a skin repo and still work (Please don't do this.)

Proposal

In practice it seems really unlikely that anything outside MonoBook should even have awareness of a module like skin.monobook.foo (apart from extensions providing skinStyles).

Nothing should depend on those, or queue them for loading on a page, except MonoBook's own code.

For the "main" module of a skin (e.g. skin.monobook.styles, skin.vector.styles etc.) there is another proposal already to reduce their registry footprint (T232148). That proposal would make it so that the skin module itself is internally implemented like skinStyles. That is, the module would simply be known as skin or skin.styles and be defined based on the current skin.

That takes care of the "main" module, but a skin can of course have additional modules that are sufficiently large or complex to warrant a separate bundle that is only loaded in some cases.

This task proposes that ResourceLoader only include those in the startup registry when in a request context for that skin. This would make it unreliable for anything other than MonoBook to call addModules( 'skin.monobook.something' ) (doing so would mean it only works on pages rendered in that skin, otherwise the module would not be known).

Implementation

This would effective feel similar to how illegal dependencies like startup are omitted, and personal origin modules, and the mobile/desktop target filtering (T127268).

In terms of implementation though, I imagine we'd do it the other way around. Rather than trying to filter them out at the last minute, the internal service wiring for ResourceLoader would make sure to only register them when needed in the first place.

(This avoids any need to introduce new mechanisms for tracking which module comes from which skin.json, and also avoids any fragile/confusing reliance on a naming scheme).

Event Timeline

Krinkle triaged this task as Medium priority.Jun 15 2020, 11:27 PM
Krinkle moved this task from Inbox to Accepted Enhancement on the MediaWiki-ResourceLoader board.

Change 682182 had a related patch set uploaded (by Genoveva Galarza; author: Genoveva Galarza):

[mediawiki/core@master] Remove modules from unselected skins from startup script

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

Note that this will break skins like Nimbus, which require styles from other skins (MonoBook in this particular case). Although in that particular case I don't see any difference.

Thanks @Mainframe98, I wasn't aware we had a precedent for a skin extending another skin across install boundaries (e.g. not provided by the same skin.json registry).

I'm not sure if this is something we officially support and document, but if we do, I would recommend that we require in that case that the two skins indicate this in some way through a formal relationship. For example, like so:

skin.json
	"type": "skin",
	"requires": {
		"MediaWiki": ">= 1.37.0",
		"skins": {
			"MonoBook": "*"
		}
	},

Then the registry can use this to help your users to tell them if they are missing a dependency, but also for systems like ResourceLoader to use a signal for how to process the module registry.

The patch for T236603 adds a getSkins() methods to ResourceLoaderModule. If merged, skin modules can override it to provide the current skin name, solving this issue. This also gives skins the flexibility to allow certain modules to be exposed to other skins, if needed, such as for the usecase mentioned above (for use in a dependant skin).

Change 749861 had a related patch set uploaded (by Krinkle; author: SD0001):

[mediawiki/core@master] resourceloader: Only register modules client-side if relevant to current skin

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

Change 749861 merged by jenkins-bot:

[mediawiki/core@master] resourceloader: Add Module::getSkins to skip irrelevant modules from startup

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

Now that ResourceLoaderModule::getSkins() patch has merged, possible approaches to move forward with this:

  1. Support skins key in JSON resource module definitions. This would require specifying "skins": [ "minerva" ] in each of the 21 RL modules in MinervaNeue, which probably isn't elegant.
  1. A new SkinResourceModules option, usage:

Register ALL minerva modules for minerva skin only:

"SkinResourceLoaderModules": {
    "skins.minerva.*": [ "minerva" ]
}

Register specific module for minerva skin only:

"SkinResourceModules": {
    "skins.minerva.content.styles.images": [ "minerva" ]
}

Note to self: When this is done, revisit T305779 as well, where we can then possibly automate the right skin context by picking one from the list when there's one. To thus re-use the same registration detection for the common case as will be done for this task.

@SD0001 I have a third option to consider as well. I think we can automate this based on skin.json in ExtensionRegistry/ExtensionProcessor as we know which skin registered the module. I think by default for any module registered from a skin we can limit skins to [ skin.json#name ] by default essentially. If a use case comes up to bypass this, then we can consider an FileModule option to override this to an empty array. I'm not currently aware of such a use case, however.

Krinkle removed a project: Patch-For-Review.
Krinkle added a subscriber: gengh.

Change 682182 abandoned by Genoveva Galarza:

[mediawiki/core@master] resourceloader: Exclude modules from other skins from startup registry

Reason:

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

@SD0001: Removing task assignee as this open task has been assigned for more than two years - see the email sent to all task assignees on 2024-04-15.
Please assign this task to yourself again if you still realistically [plan to] work on this task - it would be welcome! :)
If this task has been resolved in the meantime, or should not be worked on by anybody ("declined"), please update its task status via "Add Action… 🡒 Change Status".
Also see https://www.mediawiki.org/wiki/Bug_management/Assignee_cleanup for tips how to best manage your individual work in Phabricator. Thanks!