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 916526 abandoned by Jdlrobson:

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

Reason:

Abandoning until next release.

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

What is the plan for gadgets. There are hundreds of gadgets in the wild that use this boiler plate for adding to VE:

mw.loader.using( 'ext.visualEditor.desktopArticleTarget.init', function () {
	// Register plugins to VE. will be loaded once the user opens VE
	mw.libs.ve.addPlugin( function () {

(as documented in https://www.mediawiki.org/wiki/VisualEditor/Gadgets#User_script)

This relies on the fact that most gadgets weren't loaded on mobile because of the targets system, and if they were, the ext.visualEditor.desktopArticleTarget.init module would refuse to load.

Loading this module on mobile would lead to undefined behaviour.

We're providing help for gadgets on a case by case basis. Note this ticket is more ceremonial - the code executing now is the code that will be executed when this task is merged, so it looks like the damage to these gadgets has already been regrettably done as this wasn't flagged earlier in the process.

Whether users are getting undefined behaviour depends how the code is loaded. It looks like at least 193 users, provided they are using the mobile site, are loading code with this undefined behaviour (https://global-search.toolforge.org/?q=ext.visualEditor.desktopArticleTarget.init&regex=1&namespaces=2&title=.*%2Fcommon.js)

For MediaWiki namespace, there should be no problem with code loaded via MediaWiki:Common.js (as this is not run on mobile) but it will effect gadgets unless their corresponding MediaWiki:Gadget-definition is limiting the code so that it doesn't run on Minerva skin if the user has them enabled and accesses the mobile site:
https://global-search.toolforge.org/?q=ext.visualEditor.desktopArticleTarget.init&regex=1&namespaces=8&title=

In terms of existing code, if you need help fixing the existing boiler plates we have some tooling that can help with that. Just let me know what you want to replace what and what the replacement is by creating a subtask on this ticket.

This relies on the fact that most gadgets weren't loaded on mobile because of the targets system, and if they were, the ext.visualEditor.desktopArticleTarget.init module would refuse to load.

Right, this is one of the things we've intentionally been moving away from (more context about the why in T127268#2463941) to a more declarative way of writing code where code behaves like you'd expect it to. In this case the script says load ext.visualEditor.desktopArticleTarget.init so it's counterintuitive if that doesn't result in the loading of the code.

We are trying to move away from a system where gadgets only work on Vector and work across all enabled Wikimedia skins. This means creating better APIs and improving the existing skins to be compatible with those APIs, creating tasks where needed (eg. T340728) .

If this is for gadgets, it sounds like you might benefit from the skipFunction we've been talking about and I've finally created a ticket for T342567.

Here's some possible options off top of my head

1) Change the VE recommendation to avoid loading the code.

Checking the state is a good way to determine what mode you are and act accordingly. Feature detection is much more preferable if it can be done. e.g. your behaviour depends on a certain element in DOM or a certain editor being loaded.

if ( mw.loader.getState( 'ext.visualEditor.desktopArticleTarget.init' ) !== 'registered' ) {
	// Register plugins to VE. will be loaded once the user opens VE
	mw.libs.ve.addPlugin( function () {
}

2) update the addPlugin method to have an additional argument to determine what editor is being loaded and update callers.

mw.libs.ve.addPlugin = function ( pluginFn, 'mobile-editor' );

3) Update ext.visualEditor.desktopArticleTarget.init to take into account whether other modules have been loaded and exit if that's the case.

e.g. ext.visualEditor.mobileArticleTarget.init

if ( mw.loader.getState( 'ext.visualEditor.desktopArticleTarget.init')  !== 'registered' ) {
   mw.log.warn( 'Refusing to load desktop target as incompatible target has been loaded' );
   return;
}

4) Update MediaWiki:Gadget-definitions to exclude any gadgets using the existing pattern from executing on Minerva skin.

The skins option can still be used successfully to filter this code out of the mobile site. Just add:
skins=vector,vector-2022,monobook,timeless,modern,cologneblue

It does mean the gadget will not run on desktop Minerva, but there are very few users that use desktop Minerva.

Generally I discourage this as it encourages users to treat mobile as a 2nd class citizen but it's a good short term fix while you work out something better.

https://en.wikipedia.org/w/index.php?title=MediaWiki:Gadgets-definition&diff=prev&oldid=1186404868

Latest revision as of 22:31, 22 November 2023 view source thank
Izno (talk | contribs)
→‎test: start with deploying this for users. specifically target mobile as that is where collapsible things are not enabled (rather than a skin as you should generally). see also phab:T111565

+ * MakeMobileCollapsible[ResourceLoader|targets=mobile|rights=minoredit|default]|MakeMobileCollapsible.js

Change 977117 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/core@master] ResourceLoader: Drop targets system, deprecated in 1.41

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

@Jdforrester-WMF @Esanders what are we supposed to do now? I've made more than one gadget that specifically targets MobileFrontend and not desktop Minerva. MakeMobileCollapsible is a default gadget now.

Some possible solutions:

1. Do not drop the targets system.
2. Rename the Minerva skin on desktop or mobile so their name differs. (symbolic link??)
3. Deprecate desktop Minerva. (but I recently spotted a WMFer recommending someone to use it!)
4. Deprecate mobile Minerva. (we're not there yet!)
5. Make me search the hostname for /\.m\./ or something after the gadget already loaded... yuck

@AlexisJazz the gadgets system is not impacted by this change. It maintains its own targets system (T328610) which I highly discourage using (as it may go away in future), but it's there in the mean time if you need it. There are no plans to deprecate Minerva. Not sure where you got any idea that this is happening. There are efforts to align the desktop Minerva/Vector skins and mobile Minerva experiences more (e.g. making gadgets work in both places) and the removal of the targets system as outlined in this ticket is part of that.

I've made more than one gadget that specifically targets MobileFrontend and not desktop Minerva.

This is off topic, but please don't do this. This is discouraged on the gadget page for good reason. In the case of MakeMobileCollapsible, you should use skins=minerva. If there are any bugs with desktop Minerva, those are unexpected, please create a Phabricator ticket so we know about those issues and can help improve the software.

In T340802#9361598, @Jdlrobson wrote:

@AlexisJazz the gadgets system is not impacted by this change. It maintains its own targets system (T328610) which I highly discourage using (as it may go away in future), but it's there in the mean time if you need it. There are no plans to deprecate Minerva. Not sure where you got any idea that this is happening. There are efforts to align the desktop Minerva/Vector skins and mobile Minerva experiences more (e.g. making gadgets work in both places) and the removal of the targets system as outlined in this ticket is part of that.

I saw some vague suggestions (but not sure those were from developers) that Vector-2022 could be a step towards converging mobile and desktop. Nothing concrete.

I've made more than one gadget that specifically targets MobileFrontend and not desktop Minerva.

This is off topic, but please don't do this. This is discouraged on the gadget page for good reason. In the case of MakeMobileCollapsible, you should use skins=minerva. If there are any bugs with desktop Minerva, those are unexpected, please create a Phabricator ticket so we know about those issues and can help improve the software.

Hmm, I just assumed @Izno was correct when they said T340802#9354219 but upon checking it seems collapsing isn't enabled on desktop Minerva either and MakeMobileCollapsible works there as well.

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

[mediawiki/extensions/MobileFrontend@master] No need to set target anymore

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

Change 989241 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] No need to set target anymore

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

Change 977117 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: Drop targets system, deprecated in 1.41

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

Thanks James for getting this over the finish line!

In T340802#9464054, @Jdlrobson wrote:

Thanks James for getting this over the finish line!

Kudos to you, Timo, and the dozens of people that did so much work to land this improvement. :-)