Page MenuHomePhabricator

Allow using settings tour without an additional ResourceLoader module, and approve enabling for WMF wikis
Open, MediumPublic

Description

Background:

Status quo for ResourceLoader modules:

We currently register four different modules:

  • ext.globalwatchlist.specialglobalwatchlist for the OOUI/jQuery version of Special:GlobalWatchlist
  • ext.globalwatchlist.specialglobalwatchlist.vue for the Vue version of Special:GlobalWatchlist
  • ext.globalwatchlist.specialglobalwatchlistsettings a styles-only module for Special:GlobalWatchlistSettings
  • ext.globalwatchlist.getsettingserror a tiny module to complain about a user having invalid settings, loaded by either the php code for Special:GlobalWatchlistSettings if the php can't parse the existing user options, or loaded by the JavaScript code for Special:GlobalWatchlist if the JavaScript can't parse the options.

We then also sometimes register ext.guidedTour.globalWatchlistSettings if the GuidedTour extension is installed and $wgGlobalWatchlistEnableGuidedTour is enabled.

The only JavaScript that is loaded on Special:GlobalWatchlistSettings is in ext.globalwatchlist.getsettingserror, which is currently just a single file, getSettings.error.js. We need a separate ResourceLoader module for the settings page to show this error. However, that doesn't mean that it can't do other things too:

Proposal:

Replace ext.globalwatchlist.getsettingserror with ext.globalwatchlist.settingsErrorOrTour that will, when loaded, either

  • invoke the code in getSettings.error.js, or
  • require the ext.guidedTour module, and then run the tour for the settings page.

This new module will not have ext.guidedTour as a dependency in its module declaration, but instead only load that at run time if it is told to try and show the tour. This allows us to avoid needing to change the module declaration depending on if the extension is available and the configuration calls for making the tour available.

The javascript will detect whether to show a warning about the settings or to run the tour based on a javascript variable exported from the special page via OutputPage::addJsConfigVars().

Instead of the javascript for Special:GlobalWatchlist loading the error module, they will execute getSettings.error.js directly (it'll be updated to not show the error warning immediately when loaded).

Estimated performance impact:

  • We eliminate ever needing to register the tour as a separate ResourceLoader module when its enabled
  • We eliminate the need to have a hook handler for the ResourceLoaderRegisterModules hook, which I believe is called on every page view
  • We unfortunately need to load the messages for the tour even when they aren't being used, though I expect the impact of that to be negligible (we could instead load them via mw.Api.loadMessages() only when needed, like the ext.guidedTour module, but I expect that would be slower)
  • If we try to show the tour the ext.guidedTour dependency is loaded at run time via mw.loader.using which may be slightly slower (though we could get around that by preloading it via OutputPage::addModules for ext.guidedTour in the php code)

Given that we originally disabled the tour because it was registering a separate ResourceLoader module, which it would not under this proposal, I assume this would be okay with the Performance-Team, but I'd like to make sure before starting to work on it.
If this is approved, I'll spend some time improving the tour before we enable it for WMF wikis, so this task isn't about actually enabling it now, just approval to enable it later.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Krinkle triaged this task as Medium priority.Jul 26 2021, 7:00 PM

Untagging in favour of T264817.

Given that that might take a while, and that this is not mutually exclusive with that, @Krinkle would you mind still taking a look? It should be fairly straightforward to approve this imo - no extra ResourceLoader module is being added