Page MenuHomePhabricator

"Module not loadable on target" warning doesn't trigger on desktop (OutputPage mTarget defaults to null)
Closed, ResolvedPublic

Description

While investigating T170668: "Unknown dependency: wikibase.mobile" on Wikidata element pages I found that OutputPage and ResourceLoaderStartUpModule use different defaults for the resource loader target.

OutputPage uses null (private $mTarget = null;) while ResourceLoaderStartUpModule assumes desktop ($target = $context->getRequest()->getVal( 'target', 'desktop' ); in getModuleRegistrations). This causes all modules (no matter their target) to be added to a page (by OutputPage) which are then not available for loading, as ResourceLoaderStartUpModule is not adding them to the startup module.

Ways forward:

  1. Explicitly set the target, don't allow/ discourage null in MediaWiki
  2. Make OutputPage also default to desktop
  3. Change the default to null in ResourceLoaderStartUpModule as well, making all modules available everywhere per default.

I don't know when this started being a problem, but I presume it has not been like this for a long time, as T170668 has only been reported recently.

Event Timeline

hoo created this task.Jul 21 2017, 2:18 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 21 2017, 2:18 PM
Reedy added a subscriber: Reedy.Jul 21 2017, 2:20 PM

null task is null?

hoo renamed this task from OutputPage assumes null to Mismatching defaults for RL target in OutputPage and ResourceLoaderStartUpModule.Jul 21 2017, 2:26 PM
hoo added a project: MediaWiki-ResourceLoader.
hoo updated the task description. (Show Details)
hoo updated the task description. (Show Details)
hoo added subscribers: Krinkle, matmarex, Jdlrobson.

I'm not sure I understand the problem here and can only suspect this is a confusion about how the targets system works. I commented on T170668 that the wikibase.mobile module only defines a target of mobile - the default is desktop or null (which will default to desktop)

This causes all modules (no matter their target) to be added to a page (by OutputPage) which are then not available for loading, as ResourceLoaderStartUpModule is not adding them to the startup module.

Yes. This is intentional. The JS exception is thrown because you are using target inappropriately. If we didn't do this, you wouldn't know your code had a problem.

Change the default to null in ResourceLoaderStartUpModule as well, making all modules available everywhere per default.

I don't think this will do what you think it will do...?

hoo added a comment.Jul 22 2017, 10:29 AM

I'm not sure I understand the problem here and can only suspect this is a confusion about how the targets system works. I commented on T170668 that the wikibase.mobile module only defines a target of mobile - the default is desktop or null (which will default to desktop)

The problem is that null will not default to desktop in OutputPage, it doesn't warn due to invalid target if mTarget is null.

This causes all modules (no matter their target) to be added to a page (by OutputPage) which are then not available for loading, as ResourceLoaderStartUpModule is not adding them to the startup module.

Yes. This is intentional. The JS exception is thrown because you are using target inappropriately. If we didn't do this, you wouldn't know your code had a problem.

Ok, if this is by design that's fine (although some documentation about this would be nice… is this documented somewhere?). I would expect OutputPage to scream at me, and not client side RL, though.

Change the default to null in ResourceLoaderStartUpModule as well, making all modules available everywhere per default.

I don't think this will do what you think it will do...?

I did this locally briefly (disable the target filtering in the startup module) and that indeed makes all modules available for loading everywhere. I don't think this is very sensible, though.

Summed up, I can see what it's supposed to do: Warn me when adding a module inappropriate for the current target.

I think OutputPage should be doing this rather than client side RL!? If not, why does OutputPage have the warnModuleTargetFilter in the first place? For cases where target is explicitly set to mobile (because it's never explicitly set to desktop)?

hoo added a comment.Jul 22 2017, 10:33 AM

On another note: This also means that OutputPage::getTarget returns null for desktop, which might be hard to interpret.

Krinkle added a comment.EditedJul 22 2017, 10:08 PM

Ok, if this is by design that's fine (although some documentation about this would be nice… is this documented somewhere?). I would expect OutputPage to scream at me, and not client side RL, though.

See also T171180#3462557 at T171180: Gadget with target=mobile should not be enqueued on desktop (Warning from mw.loader.load)

The server-side warning has been around for longer than the client-side one. I'd say it's a fair bug that the server-side warning isn't being triggered, it should be triggered. We already trigger it for desktop-target modules loaded on mobile.

Krinkle renamed this task from Mismatching defaults for RL target in OutputPage and ResourceLoaderStartUpModule to "Module not loadable on target" warning doesn't trigger on desktop (OutputPage mTarget defaults to null).Jul 22 2017, 10:10 PM
Krinkle triaged this task as Normal priority.
Krinkle moved this task from Inbox to Confirmed Problem on the MediaWiki-ResourceLoader board.
matmarex removed a subscriber: matmarex.Jul 24 2017, 9:10 AM
Restricted Application added a project: Performance-Team. · View Herald TranscriptAug 18 2018, 6:49 AM
Krinkle changed the task status from Open to Stalled.Apr 12 2019, 2:14 AM
Krinkle changed the task status from Stalled to Open.Oct 9 2019, 8:01 PM
Krinkle closed this task as Resolved.Wed, Oct 16, 9:19 PM
Krinkle claimed this task.

Per T213242.