Page MenuHomePhabricator

Per-module data exports from the server-side in ResourceLoader (e.g. configuration)
Closed, ResolvedPublic

Description

Certain modules require config variables to work. For code that is loaded conditionally, the config is always loaded unconditionally. As a result of this problem, many config variables for MobileFrontend that are designed for ResourceLoader modules that can be run in the desktop or mobile context can be loaded in the desktop startup module.

Example

Consider the ResourceLoader module:

'foo' => [
   'scripts' => [ 'foo.js' ]
],

and the file foo.js:

alert( mw.config.get( 'wgFoo' );

The config value for wgFoo is often defined in one of many places - it could be in the skin's HTML or it could be in the startup module.
Usually, we may use of the hook onResourceLoaderGetConfigVars

However, there is a problem with this approach...
If the module 'foo' is never loaded on the page, wgFoo is loaded anywhere.

Possible solutions

Inside skin/extension.json

Ideally, we'd define a set of key value in the module itself
e.g.

'foo' => [
   'scripts' => [ 'foo.js' ],
   'config' => [
      'random-int': 200,
      'wgFoo'
    ],
],

The benefits of this, is that the names of the config are predictable. However, one downside of this approach is I cannot easily pass variables that depend on context or PHP consstants.

e.g. in MobileFrontend we have:

			'wgMFThumbnailSizes' => [
				'tiny' => MobilePage::TINY_IMAGE_WIDTH,
				'small' => MobilePage::SMALL_IMAGE_WIDTH,
			],

Inside hook

A hook could be added to all ResourceLoaderFileModules

$vars = [];
Hooks::run( 'ResourceLoaderGetModuleConfigVars', [ &$vars, $moduleName, $skin ] );

This would allow us to define anything within config, and scope it only to the modules that needs it.

if ( $moduleName === 'foo' ) {
   $vars['wgFoo'] = self:VALUE . 'px';
}

Event Timeline

Basically what I did is allow for package files to be either JS or JSON, and allow for virtual files of either type. Virtual files can be based on a callback or on a set of config vars.

Example module definition:

'foo' => [
    'scripts' => 'foo.js',
    'packageFiles' => [
        'config' => [ 'type' => 'data', 'config' => [ 'MFEditorOptions', 'MFExperiments' ] ]
    ],
    ...
]

Then in foo.js, you'd get the config as follows:

var configVars = require( './config' );
if ( configVars.MFEditorOptions.skipPreview ) {
    ....
}

A similar idea was proposed by Roan at https://gerrit.wikimedia.org/r/460953. A few things came up in code review and in conversions elsewhere.

mw.config: Future for module data?

The status quo is to extend FileModule and prepend a call to mw.config.set() or to extend FileModule to append a call to a function owned by the same module, the latter is what mediawiki.jqueryMsg and mediawiki.language do.

The latter is generally preferred for being more maintainable and developer-friendly. In particular:

  • It avoids the shared bucket of mw.config, where it is easy to conflict with other extensions, core, or even another module in the same extension. In particular because gadgets may also add keys here.
  • Uncertainty when the data is available. When spotting code like mw.config.get() a developer typically assumes (and usually correctly so) that the key is unconditionally available because mw-config was designed for use by OutputPage and StartupModule. It was not designed for temporal or conditional adding of keys within specific modules. Attaching data to an interface owned by the module means you can't have a situation where the interface is available but the data is not. For example, if you've loaded mediawiki.language to ensure mw.language is available, its internal data is always populated. The same applies to jqueryMsg, etc.
  • Public or quasi-private? Adding data to mw.config by default communicates that the data and its format are meant to be a publicly supported API. Even if you'd explicitly try to communicate elsewhere that it isn't meant for public use (e.g. just a private transport between the server and the 1 consuming module), it's atypical for config values to be non-public and will likely lead to someone using it incorrectly, or at least requiring additional cognitive overhead to realise when it isn't.

In short, it makes it easy to do the wrong thing, and hard to to the right thing.

The good news is, that the current approach (extending FileModule) isn't tied to mw.config, and fortunately, most extensions don't utilise mw.config when they do.

But, it means I'm unsure about an abstraction layer that encourages extending FileModule in way that is tied directly to mw.config.

So.. setter methods?

Well, not exactly. I did mention this it to Roan as counter-proposal. I'll outline it briefly, but we've since moved on to fitting it into T133462 instead, which I'll cover next.

Basically, my intermediary proposal was to take aforementioned declarative "config" interface and give it one extra property: a destination method. Something like this:

{
  "scripts": "src/mediawiki.example/index.js",
  "data": {
    "dest": "Example.data.set", // or "mw.config.set" to emulate the old model
    "values": {
      "foo": "wgExampleFoo",
      "bar": "ExampleHooks::getBarData"
    }
  }
}

This would encourage use of a setter, which may be better. But, it's still not great, because:

  • It forces the module to have a public setter. While this is fine in some cases, it's a decision that should be orthogonal based on what the module needs and wants to support. Having to passive receive the data could complicates the code and adds maintenance overhead.
  • It doesn't allow for the module to have data-based conditionals in its main scope. Instead, it forces the script to be self-contained and delay decisions to have data arrives. Again, this sounds like a good design principle. And would be for most cases of re-usable functionality. But for "UI" or "init"-type modules, not so much.

Virtual resources

Virtual resources, that is, resources that don't exist statically on disk (in Git).

We already have this concept at the module level (think about WikiModules that fetch code from wiki pages, and modules that dynamically generate JS code in PHP).

But, once the work on T133462 is completed, we can allow virtual resources to be referenced within client-side code. The private require() function exposed to files within a module, would have access to resources besides the entry point. These can be other JS files, JSON files, or virtual resources that return JSON-encodable values.

This approach has several benefits that I find appealing:

  • It doesn't introduce another primitive to learn from perspective of JS code. It's "just" plain require(). mw.config is also an existing primitive, but using it has the aforementioned issues.
  • It makes the data transport private by default. The module can choose to expose it if and how it wants to.
  • It is available when the entry point script executes and can be used there for conditionals etc.
  • It keeps us run-time compatible with non-MediaWiki environments without any dependencies. The virtual resources can be pre-build, mocked, stubbed, or exported to disk as needed. (They may even exist in Git, but unused for MW). Allowing for isomorphic modules used in Node and published to npm, whilst also being optimal for MW.
Krinkle renamed this task from ResourceLoader: It should be possible to define per module config variables to Per-module data exports from the server-side in ResourceLoader (e.g. configuration).Nov 28 2018, 2:33 AM
Krinkle lowered the priority of this task from High to Medium.
Krinkle edited projects, added Performance-Team; removed Performance-Team (Radar).
Krinkle moved this task from Inbox to Accepted Enhancement on the MediaWiki-ResourceLoader board.
Krinkle moved this task from Inbox to Backlog: Maintenance on the Performance-Team board.
Krinkle assigned this task to Catrope.
Krinkle moved this task from Backlog: Maintenance to Doing (old) on the Performance-Team board.

Roan and I decided on the direction of "Virtual resources".

This has since been implemented in fbbd65d2df43f522f4d / https://gerrit.wikimedia.org/r/471370.