VisualEditor fails to load if a plugin is not loadable
Open, Needs TriagePublic

Description

I noticed today that shortly after CodeMirror was disabled today, @Ryasmeen was unable to load VisualEditor in any tab that had already been opened from before. Clicking "Edit" resulted in no visible response to the user (no error message, no recovery mode).

Opening the developer console revealed the following error:

VisualEditor failed to load: Error: Module ext.CodeMirror.lib has failed dependencies

A few refreshes and some minutes later, it eventually resolved itself. This is due to three factors:

  1. The wgVisualEditorConfig.pluginModules array is defined by the "startup" module. This means it gets defined at page view time. Between loading the page (reading it, switching tabs) and actually clicking "Edit", the CodeMirror module ceased to exist (33124d9973, T172458). If this array was instead defined as part of the download that happens when you click "Edit", this bug would be a less likely to happen.        Granted, I understand why we define it ahead of time, - it allows us to make a single combined download for both the main VE code + any needed plugin modules. If the plugin list was part of the main VE download, that means the browser would have to wait for that to arrive, before it knows what else to download. The below are probably better solutions.
  2. The plugin loader considers it a fatal error when one of the plugins can't load.    Perhaps VE could be more graceful here. At least it should ignore cases when ResourceLoader comes back with state=missing for one of the top-level plugins listed. In cases where a plugin does load properly, but produces an actual error at run-time, we probably want to keep the current behaviour. (Continuing in that case might corruption due to potentially half-initialised code).
  3. The user interface does not inform users about loading problems. We've solved a similar problem with Parsoid loading in VE by providing a retry attempt. It might not be as useful to offer a retry, but at least we could acknowledge the failure in some way, e.g. with mw.notify(, {tag: error}) or some such. This would also make it easier for users to report such issues, and have details to provide.
Krinkle created this task.Aug 4 2017, 2:20 AM
Restricted Application added a project: VisualEditor. · View Herald TranscriptAug 4 2017, 2:20 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
DLynch added a subscriber: DLynch.Thu, Dec 14, 5:19 PM

3 we can do fairly easily; the rest is harder. mw.loader.using doesn't provide any sort of granular feedback on success/failure, so we'd need to change some fairly core ResourceLoader stuff and watch out for side-effects.

Krinkle added a comment.EditedFri, Dec 15, 2:56 AM

3 we can do fairly easily; the rest is harder. mw.loader.using doesn't provide any sort of granular feedback on success/failure

// add to registry client-side, but server will respond with state=missing
mw.loader.register('a-regnow-missinglater');

var modules = ['a-regnow-missinglater'];

mw.loader.using(modules).catch(function (err) {
  console.log('Failed due to: ' + err);
  modules.forEach(name => console.log(name, mw.loader.getState(name)));
});
//> a-regnow-missinglater "missing"

The exception object is descriptive, but not machine-readable. But you can iterate the original list with getState and find out what you need. Based on the task description, if all states are one of null, 'registered', 'ready' or 'missing' then things are likely to be fine and gracefully recoverable. If any other state is among them, the problem might not be as recoverable.