Page MenuHomePhabricator

Vector 2022 skin should not load user's vector.js before site's MediaWiki:Common.js
Closed, DuplicatePublicBUG REPORT

Description

On French wiki, an user reported the following error:

Exception in module-execute in module skins.vector.user:
ReferenceError: obtenir is not defined

obtenir() is a function that is defined in our MediaWiki:Common.js, and this function is very often used in users' personal JavaScripts (common.js, vector.js, etc.).

In theory a mw.loader.using('site') should be used, but there is no need for in practice (which is great as obtenir() is meant to be short and simple to use), as the MediaWiki:Common.js is always loaded before users' JavaScripts, and I think this order will always be ensured to be as so, as changing this order would break so many users' JavaScripts.

But when the user uses Vector 2022, currently the user's vector.js may be loaded before the site's MediaWiki:Common.js, causing the above error.

Here is the explanation:

  1. skin vector-2022 loads script module skins.vector.user
  2. module skins.vector.user executes class VectorResourceLoaderUserModule
  3. class VectorResourceLoaderUserModule loads the user's vector.js
  4. user's vector.js is executed, without ensuring the site's scripts have been executed beforehand.

To fix the issue, we have to ensure that if the user's vector.js is executed, it has a dependency on the site's scripts (namely: MediaWiki:Common.js and MediaWiki:Vector-2022.js, not sure of the name of the latter).

@Jdlrobson I think the issue is of high priority, as JavaScript may be broken for many users currently.

QA steps

  • Create MediaWiki:Common.js, MediaWiki:Vector-2022.js, MediaWiki:Vector.js, User:xx/vector-2022.js, User:xx/vector.js with console.log to display its name.
  • Check the console, the output order should be as above.

Event Timeline

Change 777906 had a related patch set uploaded (by Func; author: Func):

[mediawiki/skins/Vector@master] Add 'user' as a dependency of 'skins.vector.user' module

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

Thanks @Func for looking into this.

There is something I don't understand: as VectorResourceLoaderUserModule extends ResourceLoaderUserModule and completely replaces the function getPages(), aren't we missing loading some things such as $userPage/common.js and MediaWiki:Group-$group.js?

There is something I don't understand: as VectorResourceLoaderUserModule extends ResourceLoaderUserModule and completely replaces the function getPages(), aren't we missing loading some things such as $userPage/common.js and MediaWiki:Group-$group.js?

It is registered as a different module, so nothing is missed. But the ResourceLoaderUserModule class is marked as internal and should not be extended by extension or skin, this should be addressed in a follow-up.

Well, there is another issue that makes me not feel very good, we can see MediaWiki:Group-$group.js is executed after the user's js, is that intended?
I think MediaWiki:Group-$group.js is site-ish and should be executed at least just after <skin>.js. (Though this should be a different task.)

Jdlrobson added a subscriber: Krinkle.

In our errors logs there are only 3 instances of that error in the last 24hrs [1] so I am not considering this issue high priority, particularly since 2 of those errors come from legacy Vector, and one from Monobook.

My understanding is that ResourceLoader doesn't guarantee that Common.js loads before user skin JavaScript and users should not expect it to work that way. I would rather not add inconsistent behaviour for modern Vector only if that's the case.

It does seem understandable that users would want site scripts to run first but I'm not sure if there are downsides to that. @Krinkle would know better.

@Krinkle does it make sense to make user scripts depend on site scripts?

[1] https://logstash.wikimedia.org/goto/68e1a0285cf11c0ebb73566885aeb101

It is registered as a different module

Understood. It loads vector.js (from the legacy skin) in addition to the rest.

In our errors logs there are only 3 instances of that error in the last 24hrs

I would have expected much more than that. As I don't have access to the logs, are the 3 errors related to the obtenir() function?

It does seem understandable that users would want site scripts to run first

I think it would be very great. @Krinkle

I'll take a look and report my findings.

@Krinkle May I kindly ask if you have news about this?

Maybe gerrit 777906 is sufficient?
Shall we also add a site dependency to the user module?

Krinkle triaged this task as High priority.EditedJun 29 2022, 12:10 AM

Shall we also add a site dependency to the user module?

Generally, no. It is important for site to execute before user, but intentionally this is not implemented in core using a dependency; and Vector's copy of it should not either.

May I kindly ask if you have news about this?

Other interrupts and goals took precedence for a few weeks. I'll try to circle back later this week.

Krinkle added a subscriber: Jdrewniak.
Assumptions

The context for this problem:

  • T297758: the goal of Vector 22 supporting pre-existing vector.js scripts.
  • T291099: declined alternative implementation.
  • T300070: (other) bug in current implementation, loading common.js twice.
  • T305594: (this) bug in current implementation, not waiting for the site module.

As I understand it, the general goal here is for a wiki's community to generally be satisfied when we enable vector-2022. And, by the time it becomes the default, the majority of gadgets and scripts "just work". And, for those that don't, there's a page to consult with notable differences and how to accommodate them. And, editors would by then that and where such page exists (e.g. mentioned in Tech-News once early on and again after the switch, on Wikitech-l, and a small mention in a more general non-technical blog post).

As I understand it, the ambition for Vector 22 (as neatly summarised by @Jdrewniak in T234907) was "not disrupting the workflows of our most engaged community members" such that "gadgets, user scripts all continue to function." and all the while having "[no] negative performance impact".

Past

We've done this before in 2010, when we deployed the Vector skin. It later became the default over Monobook. Opt-in at first, then by default for unregistered and new accounts. And eventually for existing accounts via a one-off nudge.

Development of Vector did not include making breaking changes to Monobook. Not via Monobook code, nor via MediaWiki core in ways that affect user scripts. This was key to maintaining credibility and trust. To do differently would've nullified the appealing risk profile associated with creating Vector. Namely, that it can't damage the long tail of rich ecosystems around each Wikimedia-hosted and third-party MediaWiki site.

Anyone could explore and see how we're doing by occasionally opting in and out. One might port over a gadget or script if interested that time. Add vector to a gadgets skins filter, or accommodate Vector in any skin-specific CSS rule, or apply the same skinStyles file to both skins, or fork the skinStyles file if needed, or by copying some or all import statements from their personal monobook.js to vector.js, etc. Likewise, these opt-in actions and modifications by early adopters did not and could not affect those on Monobook, including the person switching. E.g. switch back for a while, and Monobook is still the same as when you left it.

Observations

These considerations were either not researched, not found, or chosen differently. That's fine of course, though if the latter, I suppose documenting the reasons for choosing so would give others a chance to appreciate why.

Despite that, I do notice a trend in these differences reversing.

  • (2020) Re-use preference key and Skin class, through a non-standard global skinversion option.
    • Outcome: Skin class and preference are split. Version override removed.
    • Known issues: Leaks through caches and rendering layers (T288113, T300278, T302627, T303265, T305232, T305966).
    • Note: The known issues cover but a small portion of the long tail that awaited someone able and willing to report. It relied on a service class that violated Dependency Injection principles (T242835#6064656), and actually still does today, continuing to produce a long tail of bugs that I suspect are being patched at the edge instead of at the source.
  • (Aug 2021) Re-use skinStyles entry, through fake skin keys in ResourceLoader.
    • Outcome: The skinStyles keys remained split and intuitive to use.
    • Known issues: Indefinite cost of maintaining, documenting, and teaching an unneeded system with additional required concepts to learn for every developer. Future bugs from unintended sharing when the two diverge with unequal QA coverage. Added risk and weeks of unscheduled work from building/testing something new. (T288857)
  • (Dec 2021) Re-use mw.config skin key, through overriding the value in vector-2022 to be vector.
    • Outcome: The config key is split. Config override removed.
    • Known issue: Breaks the standard API in JS for recognising the current skin. Corrupted edit previews and any other API-based features that vary by skin (T297758, T300814). Would also have affected EventLogging and CentralNotice reporting. Possibly incorrect load.php request context. And compromise the standard API to deal with differences between skins, such as required for gadget authors to support Vector 22 in the first place.
  • (Jan 2022) Re-use Skin.js and Skin.css, through appending to the site and site.styles modules, and replacing the user module. (T297758)
    • Outcome: ... perhaps scripts split and override removed?

The dates above suggest these were reactionary proposals — not a general plan to make the skins identify as one.

Known issues

The re-using of skin.js/skin.css seems problematic to me. I'll share my own experience here, but I imagine more compromises exist for other people:

  • First impression. This set me up for a bad first impression. I observe styles and integrated tools not working well, thus starting an immediate and mandatory chase to disable or fix stuff, only to arrive at a then-incomplete experience. Continued exposure uncovers more half-working tools. If you start out with a clean and unpersonalised state, with some gadgets that are skin-agnostic or already updated, that might leave a better first impression.
  • Cut dead wood. It's easier to append then to clean up. We can encourage communities to take inventory and update based on demand and ability. We've gone for a decade with little to no mandatory transition. Taking this opportunity to identify unwanted code, by proxy of choosing what to copy, allows imported scripts that are broken or unwanted, to become unreferenced. This improves performance, and also reduces security risk over the long run. It is incumbent to our ecosystem that this approach is paired with: an adequate timeline with months of consented opt-in, and the option to opt-out.
  • Incoherent. User scripts are now the only place in the platform where one applies something to Vector 19, but then affects Vector 22 as well. This is especially surprising for site-wide scripts. We lack an intuitive way to avoid or undo this effect.
  • Analysis, global search. If script names no longer correlate to a skin, this makes it difficult for maintainers to know in which skin their script is imported.
  • Deprecation, automation. So long as vector.js remains ambiguous in meaning, it's hard to know how and when something requires changes by an interface editor. And impossible to do en-mass with automation.

Besides the above principal issues, there are also technical issues:

  • Extension support. The user module is reserved name that extensions use to await the registering of hooks and plugins. Such as VisualEditor, WikiLove, and others. These don't use dependencies, as VE doesn't require user scripts. And user scripts may contain snippets that throw an error, which should not lead prevent features from loading. This doesn't work reliably in Vector 22 today.
  • Internal load order. The established load order is common.js -> skin.js, and likewise for common.css -> skin.css. This doesn't work reliably in Vector 22 today.
  • External load order. The established load order is site -> user. Similar to extension support, user scripts run regardless of site-wide scripts throwing an exception, hence no dependency but a graceful await (like Promise.finally).
  • Global scope. User scripts run in the global scope. This is applied to the user module, but in Vector 22 the variables in vector.js are undefined. E.g. put var x_foo = 123; console.log('x_foo', window.x_foo); in vector.js and you'll get foo 123 in Vector, but foo undefined in Vector 22.
Recommendation
  • Remove VectorResourceLoaderModule overrides.
  • (Maybe) Run a maintenance script to one-time copy over pages.

If we treat script loading like we treat skins everywhere else in the platform — as separate — then its straight-forward and self-explanatory where site-wide and personal customisations go.

Are we worried that those who have personal customisations can't find their script or know to copy it over? I suppose we could offer a button or nudge of sorts to one-time copy it over. E.g. a little gadget that if vector.js is non-empty and vector-2022.js doesn't exist, offer a button that copies it with mw.Api, and a "No thanks" that creates it an empty page, or remember in localStorage and ask again in a month.

If the automatic and silent way of today is preferred, we could run a little maintenance script that performs the copy once for all vector.css/js. That would be easy to do. And it leaves a paper trail of what and why. That ticks all the benefit boxes with none of the downside boxes!

This solves T297758 in that pre-existing vector.css/js site-wide and personal files are loaded. Yet, the architecture would be coherent, and editors can easily improve or remove pieces as-needed. Though, I think it'd be even better to not bother a copy, and give users agency. Let them choose this themselves, after a good first impression, with the opportunity to copy things over as they see fit, and naturally know what if anything needs further attention.

I would advocate for declining this. I'm going to work towards eliminating this behaviour over next 3 months.

It's not about Common vs Vector vs Vector-2022 etc scripts. It's about ensuring user scripts (whether common, vector, etc.) are loaded after site scripts (whether common, vector, etc.)

Change #777906 abandoned by Jdlrobson:

[mediawiki/skins/Vector@master] Add 'user' as a dependency of 'skins.vector.user' module

Reason:

Hello this is an automated message.
I am abandoning this patch as it over a year old, and is not currently in a mergeable state. This has nothing to do with the quality of the patch.

If you still care about this patch, please feel free to restore it and rebase it, and we can happily continue the conversation to help you get it merged.

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