Page MenuHomePhabricator

Allow ResourceLoader modules to be "private", bundle them with their dependent modules
Open, MediumPublic

Description

Especially in light of T202154, we have been reminded that there is a performance impact to creating separate modules purely to make code easier to organise. However, merging modules also makes them less maintainable.

Having to remember which features needed which dependencies and messages etc. either means you have to track it by hand through inline comments, or (more often, when using JSON to register modules) the information is lost. This means that when a feature is later refactored or removed, one has to manually check where dependencies and messages were used and which ones can be safely removed.

In the long term, this can probably be automated by having a PHPUnit structure test scan for calls to require() and mw.msg() and perhaps even fully automate the creation of module registries with am maintenance script (given a directory that represents a module bundle, create or update its ResourceLoader definition).

In the interim though, we could make the manual work a bit easier by allowing modules to mark themselves as private, and having the server bundle them along with the public modules that depend on them. This feature would also be useful for various other things, and would help us reduce the number of modules exposed in the startup manifest.

arrays of definitions
'moduleA' => [
    'scripts' => [ 'x.js', 'y.js' ],
    'messages' => [ 'x-title', 'y-title' ],
    'dependencies' => [ 'mediawiki.api', 'mediawiki.uri', 'moduleB' ],
],
'moduleB' => [
    'scripts' => [ 'z.js' ],
    'messages' => [ 'z-title' ],
    'dependencies' => [ 'oojs-ui-core' ],
    'private' => true,
],

Modules with 'private' => true (like moduleB in the example above) would not be listed in the startup manifest. For modules that depend on a private module (like moduleA), the dependencies listed in the startup manifest would not include the private module, but would instead include the dependencies of that private module. For example, the dependencies listed for moduleA would be mediawiki.api, mediawiki.uri and oojs-ui-core.

When a module that depends on a private module is loaded, the load.php response contains both the contents of the public module, and those of the private module(s) it depends on, as part of the same mw.loader.implement() call. For example:

load.php response for moduleA (with package modules)
mw.loader.implement( "moduleA@123abc", {
    main: "index.js",
    files: {
        "index.js": function () { .... },
        "foo.js": function () { ... },
        "config.json": { ... }
    },
    submodules: {
        "moduleB": {
            main: "index.js",
            files: {
                "index.js": function () { ... },
                "bar.js": function () { ... }
            }
        }
    }
}, { css: [ "combined CSS of moduleA and moduleB" ] }, { /*combined messages of moduleA and moduleB*/ } );

The example above assumes that both moduleA and moduleB are package modules. Non-package submodules are represented using a simple function; non-package main modules are represented using { script: function () { ... } }. The latter is not currently supported by mw.loader.implement(), but it's easy to add support for.

load.php response for moduleA (with non-package modules)
mw.loader.implement( "moduleA@123abc", {
    script: function () { ... },
    submodules: {
        "moduleB": function () { ... }
    }
}, { css: [ "combined CSS of moduleA and moduleB" ] }, { /*combined messages of moduleA and moduleB*/ } );

In the example above, both moduleA and moduleB are non-package modules, but the two styles can also be mixed (one can be a package module and the other a non-package module).

When executing such a module, the client-side loader would first execute each of the submodules, then execute the main module. For non-package modules this is straightforward; for package modules, this requires executing each submodule's entry point file separately, with a separate scoped require() function, as if it were a completely separate module. We may wish to implement this by registering the submodule as if it was a real module in the client-side registry.

Other things to keep in mind when implementing this:

  • The version hash for a public module needs to also reflect the contents of the private module(s) it depends on. This could be implemented by exporting a hash of the hashes of the module itself and its private dependencies (i.e. exportedHash(moduleA) = hash( hash(moduleA) + hash(moduleB) )), using the algorithm in ResourceLoader::makeCombinedHash().
  • That hash of hashes also needs to be considered when validating the version query parameter (ResourceLoader::makeVersionQuery())
  • mw.loader.store.set() needs to be able to serialize the submodule structure, which contains functions at a deeper nesting level than we were using before
  • Requesting a private module through addModules() should not be allowed (OutputPage::filterModules() should filter them out and warn).
  • However, requesting a private module through addModuleStyles() should be allowed. (For example, we probably want to make mediawiki.skinning.interface private and have skins depend on it, but Parsoid and the web installer do use it directly. Allowing direct loading of style-only private modules doesn't hurt.)

Event Timeline

I think this is quite important to do before we start merging any more modules.

For cases where there is useful separation to be had, I've indeed held off on merging or (in extreme cases like WikibaseClient where literally every single JS file was its own module) left inline comments to preserve the information.

Krinkle triaged this task as Medium priority.Jun 17 2019, 8:13 PM

I assume we will be able to use '@comment' attributes to label the sub-modules?

Yep definitely.

The input from ExtensionRegistry will have to whitelist something like @description in its schema, and then filter those out before passing onwards to ResourceLoader.

Catrope renamed this task from Add primitive to ResourceLoaderFileModule for combining groups of scripts & dependencies to Allow ResourceLoader modules to be "private", bundle them with their dependent modules.May 30 2020, 6:32 AM
Catrope updated the task description. (Show Details)

@Krinkle and I discussed this proposal at length today and fleshed it out some more. I've (significantly) revised the task description to reflect what we agreed on. It's different from the original proposal, but it will allow us to achieve the same things, with fewer changes to module definitions.

@Krinkle and I discussed this proposal at length today and fleshed it out some more. I've (significantly) revised the task description to reflect what we agreed on. It's different from the original proposal, but it will allow us to achieve the same things, with fewer changes to module definitions.

"Private modules" was how I originally pitched this idea to @Krinkle, so +1 from me :)

Part of the idea was to be able to import VE's modules.json (where we have slightly greater separation) with limited transformations.

Change 631563 had a related patch set uploaded (by Krinkle; author: Catrope):

[mediawiki/core@master] [WIP] ResourceLoader: Private modules

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