Gadget with target=mobile should not be enqueued on desktop (Warning from mw.loader.load)
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	TheDJ
	Jul 20 2017, 3:30 PM

Description

I've created a gadget that only targets mobile. When I enabled this gadget, I see in my browser console on Desktop:

[Error] Error: Unknown dependency: ext.gadget.MobileMaps
Error: Unknown dependency: ext.gadget.MobileMaps
sortDependencies — load.php:159:215
resolveStubbornly — load.php:160:377
load — load.php:171:140
(anonymous function) — index.php:13
startUp — load.php:79:750
onreadystatechange — load.php:80:313
	logError (load.php:177:344)
	handler (load.php:155:846)
	fire (load.php:45:128)
	fireWith (load.php:46:436)
	fire (load.php:46:488)
	track (load.php:155:634)
	resolveStubbornly (load.php:160:434)
	load (load.php:171:140)
	(anonymous function) (index.php:13)
	startUp (load.php:79:750)
	onreadystatechange (load.php:80:313)

Details

	Subject	Repo	Branch	Lines +/-
	Only queue gadget module if relevant on the current target	mediawiki/extensions/Gadgets	master	+13 -2
	RL: Apply gadgets target filter	mediawiki/extensions/Gadgets	master	+1 -1

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Open	None	T158181 Aim for workflow equivalence for MediaWiki on desktop and mobile web
Resolved	Jdlrobson	T127268 Dismantle ResourceLoader's "targets" system
Declined	None	T174538 Relaunch Geohack mobile maps gadget experiment on English Wikipedia
Resolved	TheDJ	T173283 Extra 100KB of JS loaded by mobilemaps gadget on pageview
Resolved	pmiazga	T171180 Gadget with target=mobile should not be enqueued on desktop (Warning from mw.loader.load)
Resolved	phuedx	T213242 Move OutputPage::setTarget() call from BeforePageDisplay to an earlier hook

Event Timeline

TheDJ created this task.Jul 20 2017, 3:30 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 20 2017, 3:30 PM

@TheDJ Thanks. This is actually an issue in the Gadgets extension. It always enqueues the enabled gadgets, even if they are not intended for the current target.

As part of working toward removing the targets system (T127268), we're adding some detection for when currently modules are enqueued on an incompatible target.

Server-side logging was added to the filter filter as part of T140675: Log modules enqueued with invalid module target. So I assume there will already have been warnings about this in Logstash from MediaWiki PHP.

I didn't mean to add client-side logging for bad-target loads specifically (and can't since they just appear as unknown modules after the filter). However, as part of solving T36853 I did add client-side logging for modules that failed to load because they are not registered (rMW12cca1c68e98: mw.loader: Log unknown modules in load() to console).

An unintended bonus of this is that we can also have client-side warnings for modules that "exist", but in a different target. (Although the message regrettably can't tell the difference.)

Krinkle renamed this task from Error: Unknown dependency in desktop, when using mobile only gadgets to Gadget with target=mobile should not be enqueued on desktop (Warning from mw.loader.load).Jul 22 2017, 2:06 AM

Krinkle added a parent task: T127268: Dismantle ResourceLoader's "targets" system.

Krinkle mentioned this in T171307: "Module not loadable on target" warning doesn't trigger on desktop (OutputPage mTarget defaults to null).Jul 22 2017, 10:08 PM

Just to get this clear, this is an Error exception that is being caught, so is currently harmless, but will pollute the console logging of Desktop users.

TheDJ added a parent task: T173283: Extra 100KB of JS loaded by mobilemaps gadget on pageview.Aug 19 2017, 9:18 PM

After https://gerrit.wikimedia.org/r/#/c/376036/ is live, these will be warnings and not errors, which will make this problem less disruptive.

• Niedzielski awarded a token.Oct 11 2017, 1:59 PM

• Niedzielski subscribed.

Any idea when this will be deployed to production enwiki? I'm looking forward to this improvement!

Restricted Application added a project: Performance-Team. · View Herald TranscriptJan 3 2018, 4:16 PM

Krinkle removed projects: Performance-Team, MediaWiki-ResourceLoader.Jan 4 2018, 8:25 PM

In T171180#3871758, @Niedzielski wrote:

Any idea when this will be deployed to production enwiki? I'm looking forward to this improvement!

The change mentioned by @TheDJ which changes this log message from "error" to "warning" has already been deployed to all wikis a few months ago.

However, the eventual goal of this task (don't load gadgets intended for other targets), requires modifications to the Gadget extension. That work has not been assigned yet. So for that part, it's not yet a question of deploying, but first doing the actual work.

@Krinkle, thanks!

Amire80 awarded a token.Feb 19 2018, 10:40 AM

In T127147#2073423, @Krinkle wrote:

Looks like maybe Gadgets doesn't implement the requirements for Targets completely. It added support for setting the module object properties but doesn't read it back out when adding modules to the page.

Either that, or the bug is in MediaWiki core OutputPage, where the style queue is missing support for the target restriction (the stylesheet urls lacks target=). This is one of various aspects of the Target system that wasn't fully considered when it was implemented. However this will be obsolete with T127268, so I don't expect these bugs to be addressed in core (unless volunteered).

Per T127268, I'd encourage you to plan ahead for when the targets system doesn't exist anymore.

For example, you may want to use the skins filter instead (e.g. skins=minerva).

Krinkle merged a task: T127147: Gadget definition "targets=mobile" seem to target mobile and desktop users on ur.wp.Jul 13 2018, 10:15 PM

Krinkle added a project: Performance-Team (Radar).

Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.

Krinkle added subscribers: MuhammadShuaib, Ciencia_Al_Poder, Majr and 5 others.

Not sure how Community-Tech is relevant here. What am I missing?

The project tag was copied over from T127147, which Andre tagged as such in 2016. Anyhow, per Maintainers looks like this is meant to be Contributors/Editing.

(Possible explanation: I think once Community-Tech stated that they might take a look at specific gadget issues on wikis, at that time...)

Change 449005 had a related patch set uploaded (by TheDJ; owner: TheDJ):
[mediawiki/extensions/Gadgets@master] RL: Only add gadget if it supports the target

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

Change 449006 had a related patch set uploaded (by TheDJ; owner: TheDJ):
[mediawiki/extensions/Gadgets@master] RL: Apply gadgets target filter

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

Change 449006 abandoned by TheDJ:
RL: Apply gadgets target filter

Reason:
superflous

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

I've been looking into this a bit, and the reason to me that this is a problem seems to be that there simply is no such thing as target=desktop. The only use of setTarget that I could find is by MobileFrontend and that sets 'mobile'. In all other situations, the target == NULL. (PS. what does this mean for PageOutput::filterModules ? )

Another problem I noticed is that MF uses the beforepagedisplay hook to set the target... And the gadgets are added from beforepagedisplay as well. That means that this targets patch can fail because the gadget hook is executed before the mobilefrontend.

This is a mess ;(

In T171180#4459955, @TheDJ wrote:

I've been looking into this a bit, and the reason to me that this is a problem seems to be that there simply is no such thing as target=desktop. The only use of setTarget that I could find is by MobileFrontend and that sets 'mobile'. In all other situations, the target == NULL. (PS. what does this mean for PageOutput::filterModules ? )

Which is why I made T140863 so we can fix the skins option in gadgets to enable mobile-only gadgets by specifying targets=mobile|skins=minerva (targets is necessary while gadgets/RL still supports targets).

In T171180#4459955, @TheDJ wrote:

I've been looking into this a bit, and the reason to me that this is a problem seems to be that there simply is no such thing as target=desktop. The only use of setTarget that I could find is by MobileFrontend and that sets 'mobile'. In all other situations, the target == NULL.

Yeah, in OutputPage we only warn if target is non-null and not in the module's targets list. This means in practice that we warn for desktop-only modules loaded on mobile, but not vice-versa. This is intentional per T140675, T127268, and how the target filter has always worked.

In T171180#4459955, @TheDJ wrote:

Another problem I noticed is that MF uses the beforepagedisplay hook to set the target... And the gadgets are added from beforepagedisplay as well. That means that this targets patch can fail because the gadget hook is executed before the mobilefrontend.

Aye, OutputPage's BeforePageDisplay hook seems like a much too late point for MobileFrontend to change the target. It should be set much earlier.

Krinkle changed the task status from Open to Stalled.Aug 6 2018, 1:07 AM

PeterBowman subscribed.Aug 20 2018, 9:17 PM

Amorymeltzer subscribed.Aug 24 2018, 7:49 PM

In T171180#4479853, @Krinkle wrote:

Aye, OutputPage's BeforePageDisplay hook seems like a much too late point for MobileFrontend to change the target. It should be set much earlier.

I marked this task as stalled and mentioned on what, but forgot to file a task for it. Sorry about that. Filed now, at T213242.

Krinkle moved this task from Watching to Perf recommendation on the Performance-Team (Radar) board.Jan 21 2019, 1:07 AM

Krinkle removed a project: Patch-For-Review.May 6 2019, 10:45 PM

Krinkle changed the task status from Stalled to Open.Sep 29 2019, 8:02 PM

Krinkle edited projects, added Web-Team-Backlog; removed Editing-team.

Krinkle added a project: Patch-For-Review.

Krinkle removed projects: Patch-For-Review, Web-Team-Backlog.

Change 449005 had a related patch set uploaded (by Krinkle; owner: TheDJ):
[mediawiki/extensions/Gadgets@master] Only queue gadget module if relevant on the curent target

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

gerritbot added a project: Patch-For-Review.Sep 29 2019, 8:03 PM