Page MenuHomePhabricator

Support skinScripts and languageScripts in packageFiles
Closed, ResolvedPublic

Description

Right now there's no way to express that a script file in a package module should only be loaded for a certain skin/language, or that different files should be loaded for different skins/languages. In traditional modules, this can be done with skinScripts and languageScripts. The lack of support for this blocks porting ResourceLoaderLanguageDataModule and WikimediaEvents to use package modules.

We can't use the existing skinScripts and languageScripts properties as-is, because adding script files that are run unconditionally in listing order breaks the principle of package modules (there is only one entry point file, and all other files are only loaded if and when invoked through require()), so we'll have to figure out another way.

Details

Related Gerrit Patches:

Event Timeline

Catrope created this task.Nov 28 2019, 12:24 AM
Restricted Application added a project: Performance-Team. · View Herald TranscriptNov 28 2019, 12:24 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Catrope added a comment.EditedNov 28 2019, 2:19 AM

I'll just start throwing around some ideas here:

A: Files that appear conditionally
{
    "packageFiles": [
        "main.js",
        { "name": "skinStuff.js", "file": "skinStuff/vector.js", "skins": [ "vector" ] },
        { "name": "skinStuff.js", "file": "skinStuff/minerva.js", "skins": [ "minerva" ] },
        ...
        { "name"; "languageStuff.js", "file": "languageStuff/nl.js", "languages": [ "nl" ] },
        { "name": "languageStuff.js", "file" "languageStuff/de.js", "languages": [ "de", "de-formal" ] },
        ...

This has a number of issues:

  • The same file name appears multiple times, which is confusing. If there are multiple conflicting definitions of the same file (that would both match the same skin/language context), we'd have to throw an error.
  • For some skin/language contexts, a file name might not have defined contents at all. The simplest way to deal with this would be to define it as empty in those cases, so that require( './skinStuff.js' ) wouldn't fail (and would be a no-op).
  • This format makes it more difficult to express things like "for Minerva use file X, for any other skin use file Y". The current system allows this using "skinScripts": { "default": "Y", "minerva": "X" }, but in this format we'd either have to come up with a way of expressing "all skins/languages except X, Y and Z", or support "skin": "default"
B: Files with multiple file definitions
{
    "packageFiles": [
        "main.js",
        {
            "name": "skinStuff.js",
            "file": {
                "skin": {
                    "vector": "skinStuff/vector.js",
                    "minerva": "skinStuff/minerva.js"
                }
            }
        },
        {
            "name": "languageStuff.js",
            "file": {
                "language": {
                    "nl": "languageStuff/nl.js",
                    "de": "languageStuff/de.js",
                    "de-formal": "languageStuff/de.js"
                }
            }
        }

This is a little closer to the current skinScripts/languageScripts model, so it's easier to see how things like "default" would map to this.

C: Callbacks

This is an extension of an idea that @Krinkle came up with:

13:05:36 <Krinkle> yeah, a virtual file with a free-form condition in the closure would be my preference.
13:05:42 <Krinkle> but it feels wwrong to do a file_get_contents there
13:05:52 <Krinkle> esp with regards to change detectin and perf.
13:06:02 <Krinkle> perhaps some way to declare that without actually doing it
[
    'packageFiles' => [
        'main.js',
        [ 'name' => 'skinStuff.js', 'callback' => function ( ResourceLoaderContext $context, Config $config ) {
            if ( $context->getSkin() === 'minerva' ) {
                return new ResourceLoaderFileHandle( 'skinStuff/minerva.js' );
            } elseif ( $context->getSkin() === 'vector' ) {
                return new ResourceLoaderFileHandle( 'skinStuff/vector.js' );
            }
            return '';
        } ],
        [ 'name' => 'languageStuff.js', 'callback' => function ( ResourceLoaderContext $context, Config $config ) {
            if ( $context->getLanguage() === 'nl' ) {
                return new ResourceLoaderFileHandle( 'languageStuff/nl.js' );
            } elseif ( $context->getLanguage() === 'de' || $context->getLanguage() === 'de-formal' ) {
                return new ResourceLoaderFileHandle( 'languageStuff/de.js' );
            }
            return '';
        } ],

ResourceLoaderFileHandle would be a new class that acts as a lazy-evaluated file, and that ResourceLoader's infrastructure would handle just as it would a "real" file for the purposes of change detection etc. To make this work well, we need the file paths to be relative to the module's base path. We could handle that by either injecting a ResourceLoaderFileHandleFactory into the callback and letting it generate the file handle objects (return $fhFactory->create( 'foo/bar.js' )), or we could let the callback do something like return new ResourceLoaderRelativeFileHandle( 'foo/bar.js' ) and have our infrastructure remap that to a ResourceLoaderFileHandle with an absolute path.

I'm not sure yet which one I prefer. I think that A is messy and confusing, but that B and C would probably work. I like declarative things generally, so I like B for that reason, but I also like the flexibility of C. (They're also not mutually exclusive, we could support both.)

If it is feasible to implement B atop C then we could start with C and add the shortcuts if/when there are frequent use cases. (In spirit of the "Extensible Web" manifesto).

Regarding file paths, yeah, looks like this might be a good use case for the ResourceLoaderFilePath class we have, which we don't use much currently, but we could have that pre-set to be scoped to the current module indeed.

Alternatively, we might also want to instead accept absolute file paths only (e.g. __DIR__ ). This because no longer need to map relative file paths twice (only FS now, not for web / legacy debug mode). For extension.json we will still support relative paths (relative to extension.json) but that's already handled by ExtensionProcessor and not something RL would need to worry about.

Change 556485 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/core@master] ResourceLoader: Allow packageFiles callbacks to return a file

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

Krinkle triaged this task as Medium priority.Jan 6 2020, 10:48 PM
Krinkle moved this task from Inbox to Accepted Enhancement on the MediaWiki-ResourceLoader board.

Change 556485 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Allow packageFiles callbacks to return a file

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

Krinkle closed this task as Resolved.Tue, Feb 4, 8:09 PM
Krinkle assigned this task to Catrope.
Krinkle moved this task from Untriaged to Archive on the Performance-Team-publish board.