Page MenuHomePhabricator

Support listing modules in extension.json that are registered if another extension is installed
Open, LowPublic

Description

We have a number of extensions with soft cross-repo dependencies; however, there's no way of implementing this without non-performant hacks via a Hook which (a) suck and (b) are out of step with all our other code going into extension.json. It'd be nice to have a way to express two levels of dependency – hard, where if the other repo isn't install nothing can work, and soft, where if so just skip the tests.

Main use case is developers working on one or two extensions not needing a dozen others to be installed.

We should also look to enforce this via code rather than socially by deprecating the ResourceLoaderRegisterModules hook and its existing users and handle this in extension loading on extension.json

Event Timeline

Jdlrobson updated the task description. (Show Details)Feb 24 2016, 9:23 PM
Jdlrobson added a subscriber: Legoktm.

Change 316508 had a related patch set uploaded (by Legoktm):
POC: Support static conditional dependencies in RL modules

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

I think this task as titled is overly general, and we're better of focusing on the specific case of conditionally registering ResourceLoader modules, and solve it in that place.

Krinkle renamed this task from Be able to express cross-repo dependencies for unit tests rather than have them fail when not all extensions are installed to Support registering modules in extension.json iff another extension is installed.Jul 7 2017, 5:24 AM
Krinkle added subscribers: hashar, TheDJ, Krinkle.
Jdforrester-WMF renamed this task from Support registering modules in extension.json iff another extension is installed to Support listing modules in extension.json that are registered iff another extension is installed.Jul 7 2017, 4:01 PM
Jdlrobson added a project: Readers-Web-Backlog.
Jdlrobson moved this task from Backlog to Watching on the Readers-Web-Backlog (Tracking) board.
Restricted Application added a project: Performance-Team. · View Herald TranscriptJun 26 2018, 5:06 PM
Restricted Application added a project: Growth-Team. · View Herald TranscriptJul 18 2018, 12:32 AM
SBisson moved this task from Inbox to External on the Growth-Team board.Jul 18 2018, 10:37 AM
Deskana renamed this task from Support listing modules in extension.json that are registered iff another extension is installed to VisualEditor.Aug 30 2018, 1:32 PM
Deskana renamed this task from VisualEditor to Support listing modules in extension.json that are registered iff another extension is installed.Aug 30 2018, 1:36 PM
Nirmos renamed this task from Support listing modules in extension.json that are registered iff another extension is installed to Support listing modules in extension.json that are registered if another extension is installed.Oct 30 2019, 12:02 PM

Getting this again out of the history again :D How would such a functionality look like from the API point of view?

Currently a reqourceloader module in extension.json looks like this:

"ext.PasswordlessLogin.login": {
	"dependencies": [
		"oojs-ui-core.styles",
		"mediawiki.api"
	],
	"scripts": [
		"ui/login.js"
	],
	"targets": [
		"mobile",
		"desktop"
	]
}

Do you think just adding a new property to the definition object would be enough and ok to achieve this feature? Something like this (see the first property of the RL module definition):

"ext.PasswordlessLogin.login": {
	"requires": {
		"Echo": "*"
	},
	"dependencies": [
		"oojs-ui-core.styles",
		"mediawiki.api"
	],
	"scripts": [
		"ui/login.js"
	],
	"targets": [
		"mobile",
		"desktop"
	]
},

And then applying the same logic as we already have on "requires", just that we do not throw an exception, but remove this module from the definition?

Three questions that arrive here:

  1. (not that important): Should we remove the additionally property when passing the definition to the RL config?
  2. The code of the extension will most likely still add this module to the output, isn't it? What are we doing then? (I actually don't know, if RL throws an exception if a non-existing module is added to the OutputPage)
  3. What if another module depends on the conditional one, but isn't conditional with the same rules by themself? Is it still being able to ue it (while it will break, if it is used, as RL can not fulfill the requirements?)

Or: Do I completely misunderstood the whole idea here?

Three questions that arrive here:

  1. (not that important): Should we remove the additionally property when passing the definition to the RL config?

Yes.

  1. The code of the extension will most likely still add this module to the output, isn't it? What are we doing then? (I actually don't know, if RL throws an exception if a non-existing module is added to the OutputPage)

It should refuse to load it as the dependency isn't met… but I think it'll throw a JS error right now?

  1. What if another module depends on the conditional one, but isn't conditional with the same rules by themself? Is it still being able to ue it (while it will break, if it is used, as RL can not fulfill the requirements?)

If you mean A (deps) B (reqs) C, and extension C is not loaded, then logically B can not nor can A, yes.

hashar removed a subscriber: hashar.Mar 30 2020, 3:35 PM

Change 316508 abandoned by Legoktm:
POC: Support static conditional dependencies in RL modules

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

In Gerrit @Krinkle wrote:

I'd prefer this be something specific to the extension.json syntax and its processor. Not part of the ResourceLoader configuration API. The coupling makes it too tight imho.
As for the actual definition key, I think something that contains 'conditional' would be clearer. "Dependencies" and "registries" are fairly overloaded in this context, already.
The downside of "registrationDependencies" is that it would duplicate module names (once in "dependencies" and once in "registrationDependencies"). And it allows for the possibility to be conditional on something that isn't a dependency, which maybe we don't want/need?

Ideas:

  • New extension registry key for conditional modules. E.g. "ExtraModules" or "PeerModules". Similar to ResourceModules but supports an extra key for registration dependencies. Like the current patch's registrationDependencies. To make it more explicit that this is a registry feature, we could add nesting to this one (e.g. ExtraModules[moduleName] would contain 'require' and 'definition' keys)
  • New extension registry key for module conditions. E.g. ResourceModulePeers would map ResourceModules keys to a list of one or more module names that must be registered first.
  • New module definition property "conditional" (boolean) in ResourceLoader. This would only register the module if all dependencies are available. This "All or nothing" approach may be too easily misused and may cause false positives if something is missing that wasn't meant to be an optional dependency.
  • New module definition property "registerWhen" in ResourceLoader. Skip registration if some are missing. Like the current patch's registrationDependencies.

It may be interesting to consider naming the key to end in "Dependencies" and merge it into that array. This avoids duplication and may avoid mistakes where something is forgotten to be removed. The downside is that it would require a conditional module to be a dependency. Which may not be the case if the relevant module is not directly interacted with, or meant to be lazy-loaded for example.

Spelled out example of what these could look like.

1 - Separate definition
{
  "ResourceModules": {
     "ext.foo": {}
  },
  "OptionalResourceModules": {
     "ext.foo.echo": {
        "requires": "Echo"
         …scripts…
      }
  }
}
2 - Inline definition with separate flag
{
  "ResourceModules": {
     "ext.foo": {},
     "ext.foo.echo": {
         …scripts…
      }
  },
  "OptionalResourceModules": {
     "ext.foo.echo": {
        "requires": "Echo"
      }
  }
}
3 - Inline definition with bool flag
{
  "ResourceModules": {
     "ext.foo": {},
     "ext.foo.echo": {
        "optional": true,
        "dependencies": [ "ext.echo.dm" ]
         …scripts…
    }
  }
}
4 - Inline definition with require flag
{
  "ResourceModules": {
     "ext.foo": {},
     "ext.foo.echo": {
        "requires": "Echo"
         …scripts…
    }
  }
}

Would require flag be string|string[] like other dependencies? If yes, option 4 is the one I like most.

Tgr added a subscriber: Tgr.Jun 2 2020, 11:25 PM

We should also look to enforce this via code rather than socially by deprecating the ResourceLoaderRegisterModules hook and its existing users and handle this in extension loading on extension.json

Dependent modules are not the only use of that hook, though. To only name the most obvious one, Gadgets uses it to register a dynamic set of modules.

Spelled out example of what these could look like.

1 - Separate definition
{
  "ResourceModules": {
     "ext.foo": {}
  },
  "OptionalResourceModules": {
     "ext.foo.echo": {
        "requires": "Echo"
         …scripts…
      }
  }
}

This is my preferred option, with a slight twist I'll propose as #5. I like how the optional modules are clearly separated. I dislike that the requires property is nested inside the module definition. But it's not actually part of the module definition, it's part of the ExtensionRegistry system.

5 - Separate definition more like attributes
{
  "ResourceModules": {
     "ext.foo": {}
  },
  "OptionalResourceModules": {
    "Echo": {
      "ext.foo.echo": {
         …scripts…
      }
    }
  }
}

This looks more like the attributes v2 syntax as well. Downside: you can only depend on a single extension/skin, which James wants.

2 - Inline definition with separate flag
{
  "ResourceModules": {
     "ext.foo": {},
     "ext.foo.echo": {
         …scripts…
      }
  },
  "OptionalResourceModules": {
     "ext.foo.echo": {
        "requires": "Echo"
      }
  }
}

The duplication is weird, I think the dependency should be kept with the definition.

3 - Inline definition with bool flag
{
  "ResourceModules": {
     "ext.foo": {},
     "ext.foo.echo": {
        "optional": true,
        "dependencies": [ "ext.echo.dm" ]
         …scripts…
    }
  }
}

I'm OK with this, I think the implementation might be a little weird if you end up with a chain of optional modules or something.

4 - Inline definition with require flag
{
  "ResourceModules": {
     "ext.foo": {},
     "ext.foo.echo": {
        "requires": "Echo"
         …scripts…
    }
  }
}

At first I was against this because it requires iterating over every module definition to see whether it has a dependency or not, and then I remembered that we already do that anyways for path expansion.

Would require flag be string|string[] like other dependencies? If yes, option 4 is the one I like most.

How important is it to depend upon multiple extensions? IIRC all the cases we ran into were just depending upon 1 extension.

Jdforrester-WMF added a comment.EditedJun 5 2020, 12:02 AM

Would require flag be string|string[] like other dependencies? If yes, option 4 is the one I like most.

How important is it to depend upon multiple extensions? IIRC all the cases we ran into were just depending upon 1 extension.

Dual dependencies exist, e.g. WikibaseCirrusSearch (hard-)depends on both Wikibase and CirrusSearch.

Off the top of my head:

Krinkle added a comment.EditedJun 5 2020, 1:31 AM

I believe @Legoktm is correct that we do not currently have use cases involving resource module registration where something hard-requires two or more extensions. E.g. WikibaseCirrusSearch's modules can always assume both. Cite and Flow might want to efficiently register one bundle if either of its soft-deps is present (a new theoretical use case), but I'm not sure it has a single module that would require two extensions to be useful.

My instinct says that if someone were to be in a situation where a particular file is only needed if two dependencies, that are optional to the extension but hard-required for one of its JS files to be useful, that this is likely a situation that should not be using ResourceLoader modules to structure its dependency tree, but rather it is so complex that the entire definition likely needs to be a single module composed by packageFile callbacks or PHP conditionals.

I also think @Jdforrester-WMF is correct in that these are not unlikely to arise at some point, and I don't think we should structure our interface in such a way that would require an overhaul and nasty migration just to allow more than 1 dependency.

I'm neutral on whether to support multiple from the get to, I would support if e.g. our schema requires a string, which we could later lax to allow an array of strings if and when a non-footgun use case comes up.