Change 885793 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/core@master] ResourceLoader: Define targets in a single location

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

Not doing this for ResourceLoaderModule was somewhat intentional to limit scope and impact of the change - it seems in the PageTriage case, a RL\FileModule is depending on a RL\WikiModule (which I believe is quite a rare situation). The change in https://gerrit.wikimedia.org/r/885793 would change the defaults for WikiModules as well as anything using Module directly.

In terms of Module: it is only used in https://codesearch.wmcloud.org/deployed/?q=extends%20ResourceLoaderModule&i=nope&files=&excludeFiles=&repos=
I'm not too worried about this, as I think the only thing this would effect is CurrencyRatesModule (the others explicitly set targets)

In terms of WikiModule however I see this as pretty risky. Codesearch returns these impacts modules as well as all the core styles/scripts modules.

https://gerrit.wikimedia.org/g/mediawiki/extensions/WikiLove/+/43fe2a66447de583fdc2bb38bfec1f306908f4ab/extension.json#222
and https://gerrit.wikimedia.org/g/mediawiki/extensions/Gadgets/+/a7523dca5638a89cf0ad035201e71cd664920549/includes/GadgetResourceLoaderModule.php#12

The latter seems pretty risky, since it could lead to a lot of untested gadget code being unleashed on mobile so I think if we do the core change, we should also cautiously set gadgets (see also reply to wikitech-l thread)

I think we should eventually do this core change, but should likely make this happen in a later iteration.

I'd therefore advise we do the following:

1. Merge https://gerrit.wikimedia.org/r/c/mediawiki/extensions/PageTriage/+/885881 that should unblock code changes in the PageTriage extension. Identify other issues in our our code.
2. Let next weeks' train roll out. Monitor logs for any other problems.
3. Run a User-notice about https://gerrit.wikimedia.org/r/885793
4. Merge https://gerrit.wikimedia.org/r/885793 when user notice has been seen and community have had enough time to comment.

How does that sound?

In T127268#8580111, @Jdlrobson wrote:

Not doing this for ResourceLoaderModule was somewhat intentional to limit scope and impact of the change - it seems in the PageTriage case, a RL\FileModule is depending on a RL\WikiModule (which I believe is quite a rare situation). The change in https://gerrit.wikimedia.org/r/885793 would change the defaults for WikiModules as well as anything using Module directly.

In terms of Module: it is only used in https://codesearch.wmcloud.org/deployed/?q=extends%20ResourceLoaderModule&i=nope&files=&excludeFiles=&repos=
I'm not too worried about this, as I think the only thing this would effect is CurrencyRatesModule (the others explicitly set targets)

In terms of WikiModule however I see this as pretty risky. Codesearch returns these impacts modules as well as all the core styles/scripts modules.

https://gerrit.wikimedia.org/g/mediawiki/extensions/WikiLove/+/43fe2a66447de583fdc2bb38bfec1f306908f4ab/extension.json#222
and https://gerrit.wikimedia.org/g/mediawiki/extensions/Gadgets/+/a7523dca5638a89cf0ad035201e71cd664920549/includes/GadgetResourceLoaderModule.php#12

The latter seems pretty risky, since it could lead to a lot of untested gadget code being unleashed on mobile so I think if we do the core change, we should also cautiously set gadgets (see also reply to wikitech-l thread)

I think we should eventually do this core change, but should likely make this happen in a later iteration.

I'd therefore advise we do the following:

1. Merge https://gerrit.wikimedia.org/r/c/mediawiki/extensions/PageTriage/+/885881 that should unblock code changes in the PageTriage extension. Identify other issues in our our code.
2. Let next weeks' train roll out. Monitor logs for any other problems.
3. Run a User-notice about https://gerrit.wikimedia.org/r/885793
4. Merge https://gerrit.wikimedia.org/r/885793 when user notice has been seen and community have had enough time to comment.

How does that sound?

That sounds reasonable, thank you for thinking through the implications of that for gadget code.

Okay I've prepared for 3. I'll make sure that happens at earliest opportunity.

@Catrope has pointed out that gadgets will not be impacted by this change because the targets definition comes from https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/Gadgets/+/a7523dca5638a89cf0ad035201e71cd664920549/includes/Gadget.php#62

So actually the core change should be relatively safe - only CurrencyRatesModule and Wikilove. I have therefore rescinded my review.

Change 885788 merged by jenkins-bot:

[mediawiki/core@master] Adjust default targets in RL/Module

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

Change 885793 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: Define targets in a single location

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

Change 888816 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] WIP: Error for any module that introduces bad targets

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

Change 271462 abandoned by Jforrester:

[mediawiki/core@master] resourceloader: Include mobile in default FileModule targets

Reason:

Done in Ia062ff2d8b8732b0d3498c1a614bbf6a3e3a7ddb.

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

Change 906135 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Load mediawiki.page.gallery on mobile

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

Change 906135 merged by jenkins-bot:

[mediawiki/core@master] Load mediawiki.page.gallery on mobile

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

Change 916526 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Dismantle ResourceLoader's "targets" system

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

Change 888816 merged by jenkins-bot:

[mediawiki/core@master] Tests: Error for any new module that introduces bad targets

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

Hey @Krinkle I could do with your expertise here. Many third parties are using the targets system so I want us to give them time to update, but since Wikimedia production is not using the targets system I want us to reap the benefits.

How would you recommend we deprecate the targets system in a way that we can turn it off for Wikimedia production, but allow 3rd parties to continue to use it with deprecation warnings for one release cycle?
Would it be feasible to have a configuration flag $wgUseTargetsSystem that is true by default and disabled in production or is there a better way we could do this?

Thanks in advance for any thoughts you have.

In T127268#8916541, @Jdlrobson wrote:

How would you recommend we deprecate the targets system in a way that we can turn it off for Wikimedia production, but allow 3rd parties to continue to use it with deprecation warnings for one release cycle?

Isn't that the point of deprecation warnings in general, that sites that don't care can just ignore them and sites like Wikimedia can treat them as production errors? I don't think a configuration flag would add anything to that. A flag is useful if you want to disallow something on the Wikimedia cluster but support it elsewhere for the foreseeable future.

@Tgr I'm asking if we can remove the use of targets filtering in Wikimedia production e.g. Minerva desktop site would share a ResourceLoader cache with Minerva mobile site. That's a breaking change that fundamentally changes how code is loaded.

If we remove it now, for third parties that havent updated their extensions: mobile code would load on desktop and vice versa which would likely break things. We can trigger deprecations for the presence of targets but if the targets system has been removed those deprecation warnings are meaningless as the damage has already been done to the wiki.

To take a concrete example: the ImageCompare extension has several modules that are meant for mobile only
https://gerrit.wikimedia.org/g/mediawiki/extensions/ImageCompare/+/208dc18907ee889cf548d51e2c580ef79eb15ffe/extension.json#37
While we can send deprecation notices for those modules to the wiki owner/developer, if the targets system is removed, those modules will now load on mobile and desktop views (presumably with errors in the former) which would be a breaking change.

The easiest thing is to keep the targets system as is and send deprecation warnings on modules defining targets but it would mean Wikimedia sites are not getting any potential ResourceLoader cache benefits for 2 or 3 release cycles.

Does that help explain why I am asking this question?

In T127268#8919340, @Jdlrobson wrote:

[…] Minerva desktop site would share a ResourceLoader cache with Minerva mobile site. […]

Which cache are you thinking of in this context?

Change 929795 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Deprecate use of targets

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

Change 929795 merged by jenkins-bot:

[mediawiki/core@master] Deprecate use of targets

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

Change 931955 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Gadgets@master] Gadgets should default to mobile and desktop targets

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

Change 931955 merged by jenkins-bot:

[mediawiki/extensions/Gadgets@master] Gadgets should default to mobile and desktop targets

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

Gadgets will now load on both desktop and mobile by default. Previously, gadgets loaded only on desktop by default. You can still change the default using the |targets= parameter in MediaWiki:Gadgets-definition, but the parameter will be removed shortly (T328610). If possible, make gadgets work on mobile or disable them based on the skin (|skins= parameter) rather than whether the user uses the mobile or the desktop website.

ou can still change the default using the |targets= parameter in MediaWiki:Gadgets-definition, but the parameter will be removed shortly

This will trigger production error logs, so this shouldn't be encouraged. Basing on the skin is preferable.

All the MediaWiki:Gadget-definitions have been patched so there is no action necessary with this change so I guess if this is now running as a user-notice this would be an FYI (e.g. Don't use targets for gadgets in MediaWiki:Gadget-definitions)

https://global-search.toolforge.org/?q=targets%5C%3D%28mobile%7Cdesktop%29%5C%7C&regex=1&namespaces=&title=Gadgets-definition

In T127268#8954406, @Jdlrobson wrote:

This will trigger production error logs, so this shouldn't be encouraged.

How? Looking at the above patch, it adds no error messages.

Basing on the skin is preferable.

I agree that basing on the skin is preferable, but I think it’s acceptable to use |targets= as a very-short-term temporary solution. If you have a wording that makes this clearer, I’m open to changing the message.

All the MediaWiki:Gadget-definitions have been patched so there is no action necessary with this change so I guess if this is now running as a user-notice this would be an FYI (e.g. Don't use targets for gadgets in MediaWiki:Gadget-definitions)

https://global-search.toolforge.org/?q=targets%5C%3D%28mobile%7Cdesktop%29%5C%7C&regex=1&namespaces=&title=Gadgets-definition

https://hu.wikipedia.org/wiki/MediaWiki:Gadgets-definition, for example, hasn’t been patched: there are many gadgets that don’t specify targets, which means that they will suddenly start loading on mobile, and potentially break there. It’s clearly something that needs attention from gadget maintainers, not just a FYI – they need to check all gadgets on mobile and fix/disable those that don’t work.

Re: Tech News - For additional context, there was a Tech News entry about this in February -- It read:

Gadgets and user scripts will be changing to load on desktop and mobile sites. Previously they would only load on the desktop site. It is recommended that wiki administrators audit the [[MediaWiki:Gadgets-definition|gadget definitions]] prior to this change, and add skins=… for any gadgets which should not load on mobile. More details are available.

Does that help to write an updated entry? Thanks!

https://hu.wikipedia.org/wiki/MediaWiki:Gadgets-definition, for example, hasn’t been patched: there are many gadgets that don’t specify targets, which means that they will suddenly start loading on mobile, and potentially break there.

I'll be monitoring the error logs and we'll know quickly and get those patched where needed, but yes I suppose if gadget developers want to intentionally disable Minerva via skins that is fine.

How? Looking at the above patch, it adds no error messages.

This impacted TranslateWiki recently: https://phabricator.wikimedia.org/T340046 - in short a log message will be triggered for any module which is enabled on only mobile or only desktop.

Edit: So in short setting targets will trigger production warnings and for popular gadgets could result in the train being rolled back or stalled so please don't encourage that.

So if I understand correctly, any use of targets that is different from ['desktop', 'mobile'] is deprecated since be122d4f2ad4, including relying on the default for gadgets being desktop – it’s just that be122d4f2ad4 didn’t hit production WMF wikis yet due to the no-train week. Am I right? If yes, that’s unfortunate – there should have been some time between changing the default and making the old default unusable. This was the case for modules defined by extensions, but isn’t the case for gadgets.

Actually, it’s not too late: the Gadgets extension could be modified (temporarily) to interpret targets on its own: define the ResourceLoader modules without targets (i.e. both on mobile and on desktop), avoiding the logspam, but load gadgets only according to their targets parameters (if any). This would still cause subtle differences compared to the status quo (gadgets loaded using mw.loader.load('ext.gadget.*') would load instead of erroring out), but it would leave time to gadget maintainers to migrate somewhat at their own pace in general cases.

In any case, if I understand correctly, the message should be quite different:

Gadgets will now load on both desktop and mobile by default. Previously, gadgets loaded only on desktop by default. Changing this default using the |targets= parameter in MediaWiki:Gadgets-definition is also deprecated and should not be used. You should make gadgets work on mobile or disable them based on the skin (|skins= parameter) rather than whether the user uses the mobile or the desktop website. Popular gadgets that throw errors on mobile will be disabled by developers on the Minerva skin as a temporary solution.

(By the way, I don’t think the logspam amount depends on the popularity of a gadget – AFAIK, ResourceLoader modules of all gadgets are registered for everyone, which is why gadgets can be loaded using mw.loader.load. So however few people use a gadget, if it’s defined on a large wiki with |targets=desktop, it will cause a huge amount of logspam.)

If yes, that’s unfortunate – there should have been some time between changing the default and making the old default unusable.

This was previously announced on tech news in February 2023 (T328610).

The February announcement contained no timeline apart from “soon”. Even if volunteer gadget maintainers did read Tech News, they had no idea by which time they have to fix the gadgets. And probably many of them didn’t even read it, so the first time they realize it’s going to stop working is when the default changes and users start to report broken gadgets. At this point, they need time to fix them.

"soon" here were four months. So there were four months "time to fix them". If folks "didn't even read it", why would they read yet another warning? This doesn't help

In T127268#8962713, @Aklapper wrote:

So there were four months "time to fix them".

But nobody said before that it will be four months, and not, for example, half a year or a year.

If folks "didn't even read it", why would they read yet another warning?

Because this time the “warning” is not in Tech News, but in the fact that gadgets using the default break. I’m not advocating against changing the default, I’m advocating against making it practically impossible on WMF wikis to set anything but the new default (due to non-default values causing a huge amount of logspam).

I’m sorry, but the suggestion to disable gadgets based on skins is weird. Minerva can be used on desktop, too, where it can make sense to load gadgets while not loading them on mobile (if it is, for example, a default gadget that should work in all skins, but should not be shipped to mobile website because of its size, for example). I thought the whole point of the mobile version was that there is less code shipped there by default, now you are removing the way to ship less code that was written by the volunteer devs?

@stjn agreed. I don't think any gadgets should be based on skins. The proper fix here is to update the gadget or introduce a gadget loader to restrict the loading of code. The skins filtering should only be used as a stopgap solution where a gadget developer needs more time.

I thought the whole point of the mobile version was that there is less code shipped there by default, now you are removing the way to ship less code that was written by the volunteer devs?

No that was never the intention. The intention was always to allow time for code to be adapted to work on the mobile site. It was a temporary message which stayed around longer than expected. We want to ship less code on desktop as well as mobile. There are people on desktop devices using slow/unsteady connections as well as on mobile devices.

A gadget can still organize itself in such a way that it minimizes code shipped by separating itself into 2 modules e.g. "ext.gadget.foo.loader" and "ext.gadget.foo.core". The bulk of the code should be in ext.gadget.foo.core and pulled in if and when needed.

To give a concrete example - ext.gadget.foo.loader might do a check like this:

if ( window.innerWidth > 1000 ) {
   $( '.some-css-selector' ).on( 'click', () => mw.loader.using( 'ext.gadget.foo.core' ) );
}

Or if you prefer a real-world example, here's one that limits where code is loaded based on the current page: https://en.wikipedia.org/wiki/MediaWiki:Gadget-extra-toolbar-buttons.js

I am glad we are in agreement on skins= option being the wrong suggestion. Still,

1. Unless someone at the WMF does the work of checking every default gadget to make sure they don’t load unnecessarily large code on mobile, this will cause increased JS payload in almost every wiki.
2. The way you are proposing to solve this is incredibly convoluted from the point of gadget developers, which don’t seem to be in this conversation at all. Gadget IDs cannot be changed (without breaking gadget prefs), so all local interface administrators should be expected to change them responsibly, which a lot of people will inevitably not do if this causes major issues and they’ll have to do this quickly.

I think this should be, at least, a change that is first visible to users, and then to everyone. Otherwise you are just going to break a bunch of things and expect everyone to pick up the scraps.

(Edit: oops, didn’t remove the first version of my comment.)

Unless someone at the WMF does the work of checking every default gadget to make sure they don’t load unnecessarily large code on mobile, this will cause increased JS payload in almost every wiki.

This has been the case for at least 1 month now and there are no problematic increased JS payloads on https://grafana.wikimedia.org/d/000000205/reading-web-performance?orgId=1&from=now-30d&to=now over the last 90 days relating to gadgets (note: there was a change relating to Vector 2022 but that is being acted on in T338911).

The way you are proposing to solve this is incredibly convoluted from the point of gadget developers, which don’t seem to be in this conversation at all. Gadget IDs cannot be changed (without breaking gadget prefs), so all local interface administrators should be expected to change them responsibly, which a lot of people will inevitably not do if this causes major issues and they’ll have to do this quickly.

I'm not seeing any significant errors in our logstash (error reporting tool) relating to the change to gadgets that was already made. The change to log warnings is purely symbolic at this point - all the work has been done since February.

I think this should be, at least, a change that is first visible to users, and then to everyone. Otherwise you are just going to break a bunch of things and expect everyone to pick up the scraps.

The change was made already and nothing appears to be broken. If you have specific examples I would love to know about them, rather than worrying about hypothetical problems.

Since I do not see gadgets like Vietnamese Wikipedia’s AVIM (see https://vi.wikipedia.org/wiki/Đặc_biệt:Tiện_ích?uselang=en) being loaded on mobile (40 Kb uncompressed, don’t know what size compressed), I can only assume this change isn’t deployed yet, no? Grafana link seems to test only English Wikipedia, which is not the only wiki (even though some English speakers might consider it the most important).

Okay I understand what you are saying now. Yes that particular change relating to default gadgets beginning to run on mobile has not gone live and yes when it does it will increase bytes for mobile users. I did go through the lists of default gadgets and noted they all ran without JS errors on Minerva desktop and I didn't see anything concerning relating to increase in payload. Even AVIM is not a considerable amount of JS. Thus I am fairly confident for this train. Despite that I'll be personally monitoring all the dashboardsand to be clear - I am accountable/responsible for addressing any issues that arise. Regarding performance - we do collect performance metrics from real users, so that was what I was referring to.

I'm sorry I misjudged the concern this would cause and thanks you for your patience. Given the concerns here I'll do some additional verification and post something more detailed later today.

In T127268#8966039, @Jdlrobson wrote:

A gadget can still organize itself in such a way that it minimizes code shipped by separating itself into 2 modules e.g. "ext.gadget.foo.loader" and "ext.gadget.foo.core". The bulk of the code should be in ext.gadget.foo.core and pulled in if and when needed.

That would bloat the startup module, which needs to include a list of all ResourceLoader modules, including all gadgets (whether or not they are enabled by default or for the current user). It's also a maintenance burden.

Maybe we should expose skipFunction to gadgets, but that's not a trivial change.

Mentioned in SAL (#wikimedia-operations) [2023-06-28T19:46:54Z] <brennen> train 1.41.0-wmf.15 (T340243): deploying a revert for T127268 related deprecation logspam - this is likely to impinge on upcoming backport window, which currently has no patches. will update when finished.

I'm still analyzing gadgets for errors and performance. More on that later.

Based on another extension we found and a few more gadgets we missed, we decided to revert the log warning message for now. This does mean gadgets can use targets for an additional week if we do discover any further blockers in tomorrow's deploy.

The rollout for Group 1 wiki went pretty smoothly from an error point of view - only 2 gadgets on Commons had issues relating to the fact that mw.config.get( 'wgCategories' ) is undefined on the mobile site.

I like the idea of @Tgr skip function - I have suggested a compromise on T328610#8974807 - let's continue conversation there.

I've been manually checking most of the wikis here: https://docs.google.com/spreadsheets/d/1OZ6JBr7_E3ZW0E9dTBw40gF6bP8HBcSWMe_DMAu_in0/edit#gid=457716643 (can share PDF snapshot if requested). Basically any gadgets that are breaking are already broken on the desktop site. The performance of some of those sites is definitely an issue but it should be viewed through the lens of desktop as well as mobile - basically mobile is now inheriting the existing performance debt of desktop on certain sites. I'll track that here: T340705

Change 938421 had a related patch set uploaded (by 沈澄心; author: 沈澄心):

[mediawiki/extensions/CookieWarning@master] Remove targets

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

Change 938422 had a related patch set uploaded (by 沈澄心; author: 沈澄心):

[mediawiki/extensions/Disambiguator@master] Remove targets

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

Change 938422 merged by jenkins-bot:

[mediawiki/extensions/Disambiguator@master] Remove targets

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

Change 938421 abandoned by 沈澄心:

[mediawiki/extensions/CookieWarning@master] Remove targets

Reason:

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

Change 961208 had a related patch set uploaded (by Winston Sung; author: Winston Sung):

[mediawiki/core@master] MainConfigSchema: Update doc for "ResourceLoader: Default File modules to mobile and desktop targets"

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

Change 961208 merged by jenkins-bot:

[mediawiki/core@master] MainConfigSchema: Update doc for "ResourceLoader: Default File modules to mobile and desktop targets"

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

Change 961214 had a related patch set uploaded (by Reedy; author: Winston Sung):

[mediawiki/core@REL1_40] MainConfigSchema: Update doc for "ResourceLoader: Default File modules to mobile and desktop targets"

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

Change 961214 merged by jenkins-bot:

[mediawiki/core@REL1_40] MainConfigSchema: Update doc for "ResourceLoader: Default File modules to mobile and desktop targets"

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