Page MenuHomePhabricator

Dismantle ResourceLoader's "targets" system
Open, LowPublic

Description

Endlessly discussed and griped about but apparently there's no task.

This system is carelessly leaving behind a trail of maintenance overhead. With new issues popping up all the time. Here's a quick sample just from MediaWiki core (quick Resources.php search for "target" and "mobile").

Fixups

Issues

Plan

Right now, the problem is that many extensions rely on using OutputPage::addModule inside hooks to add code. There is no way for mobile or a skin to override these decisions. However, many JavaScript modules assume a certain skin and would break if loaded in the wrong skin.

Existing violations can be determined by using X-Wikimedia-Debug and scanning the logs for the query "message:"not loadable on target"
e.g. https://logstash.wikimedia.org/app/kibana#/dashboard/x-debug?_g=h@7bf0c26&_a=h@f3073fc

Prep work

  • Many modules in MobileFrontend set "targets": "mobile". This is a shortcut to making sure code only loads in mobile. A module should either by desktop+mobile or desktop going forward. Fix all violations that set only mobile. The important thing to fix is "where" and how the code gets loaded. Where code needs to only load on mobile it should make use of MobileContext.
  • Error when a module is loaded without the 'desktop' target.
  • We need a mechanism for extensions to load modules in such a way that a skin can disable them (or use a different module in its place). One way might be to move all module registrations to Skin::getDefaultModules and preventing access to addModules and addModuleStyles. This would allow the skin the final say in what modules get added to the page. Is that something that's plausible?
  • Certain modules like gadgets and mobile.site are kept out of mobile by the targets system
  • We'll mark any deprecated modules that are not mobile friendly as 'deprecated' e.g. jquery.ui - while it will be possible to load these on mobile, we'll make sure these are not on the critical path of non-special page views.

Deprecating 'targets' system

  • Move all modules to the new system for module registration.
  • target mode desktop will be used on special pages going forward. This should remove many of the special page related violations.
  • When the logs are running clean, we'll turn off the targets system - all modules will be 'mobile'+'desktop' by default.

Related Objects

StatusAssignedTask
OpenNone
OpenNone
ResolvedKrinkle
Resolvedpmiazga
Resolved Jdlrobson
DeclinedNone
OpenNone
Resolved Jdlrobson
DeclinedNone
ResolvedNiedzielski
DuplicateReedy
ResolvedNiedzielski
Resolved Jdlrobson
Resolved Jdlrobson
DuplicateNone
Resolvedpmiazga
Resolvedovasileva
Resolvedovasileva
Resolvedovasileva
DuplicateNone
StalledNone
OpenNone
DuplicateNone
Resolvedpmiazga

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Dismantling targets at this stage is a lot of work. It's not a simple flick of a switch. I don't have capacity to do all the above right now, but by all means please bring this up with Toby/Adam/Jon Katz to ensure it gets attention from reading.

If you want to make progress here I'd suggest that it might be worthwhile doing the disabling of targets first as a user preference or via a special debug mode so we can assess the impact safely (to be clear I don't want to discourage investigation).

Big blockers as I see it are:

  1. Without the required care and testing this would load all user scripts in common.js on Minerva which would break the mobile site for many logged in users (most gadgets are incompatible) so needs a high level of testing/help from community liason.
  1. This would throw a lot of untested and non-mobile friendly JavaScript/CSS onto mobile devices. This is most likely to impact Special pages. I'm not sure whether it is better to have a substandard/broken experience rather than no experience at all? Usually as a rule of thumb a special page should have a basic functional non-JavaScript experience (which is better than a broken one).
  1. Will load unused code on various pages due to hook system / performance degradations

Various extensions add modules to all page views (e.g. Echo and UniversalLanguageSelector). The code for these extensions would be loaded on the mobile site and this code would be incompatible resulting in either the breakage of the mobile site or a performance bloat. We need to find a way to load modules only on certain skins.

  1. There has been a lot of work so far on reviewing code for mobile. Certain modules have been purposely blacklisted for example jquery.ui. I'd appreciate not losing this effort by implementing T137772 thus I have added as a blocking task.

In case history is interesting. See T42033

With respect to all the examples in the description - my interpretation of this is that devs are not actively testing on the Minerva skin or on mobile devices. It's thus probably a good thing targets reminds them to do so.

Many of the patches you list in core are the result of careful consideration of consequences (although various do seem to be done without thinking). On a side note, I'd appreciate all targets changes in core went through the Front-end-Standards-Group to ensure we don't enable libraries that shouldn't be - several seem to have been enabled without any discussion or mobile testing.

Krinkle added a comment.EditedJul 14 2016, 8:41 PM

@Jdlrobson I agree that we've got modules loaded unconditionally right now that shouldn't execute on mobile. I'm not suggesting we remove targets without solving that first. (That'd actively break things indeed.)

There is a sizeable amount of technical debt hiding behind this that we've been ignoring. We're loading modules that we shouldn't be loading. Right now the "targets" system is intercepting this by pretending certain modules don't exist and ignoring their load requirement.

This is counter-intuitive. When writing code that adds a module to a page, the code rightfully assumes that it will be loaded. It magically disappearing creates an unexpected relationship between core, random extensions and MobileFrontend's potential replacement of such module.

As such, various special pages are currently broken on mobile. (Though, fair enough, some of them would be equally broken if we did load the modules as they weren't written for mobile.) For special pages we don't want to invest in for mobile yet, I suggest we just let them be. Presumably MobileFrontend already makes them undiscoverable. It'd mostly be people opening them directly, in which case we should let them. Especially as our code and browsers improve over time. (If really important, MobileFrontend could remove or override the page using $wgSpecialPages and display a helpful message of sorts.)

I suggest the following steps:

  1. Emit a run-time warning if a module is added to OutputPage that isn't legal for the current request's target.
  2. Address those warnings.
  3. Remove the target system.

Addressing a warning means, making MediaWiki not load the "wrong" module in the first place. This is not always easy. Let's take a look at the mediawiki.toc as example. MediaWiki core unconditionally loads it whenever a table of contents is added to a page. There's generally three ways to deal with this, all equally valid depending on the individual case:

  • Make the module work everywhere.

    E.g. VisualEditor plugins and other code is built on top of generic interfaces that themselves already vary by skin or theme. Similarly, for cases where styles may be desktop-specific (e.g. menus only accessible via :hover), these should be improved upstream by adding focus or touch handlers. This is good for accessibility in general, and for desktop users that have touch-enabled laptops, and for mobile/tablet users using our desktop site, and third-parties without MobileFrontend, etc. We can do that without module-level separation between "desktop" or "mobile".
  • Provide skinStyles.

    If a style is fairly specific to our desktop skins (e.g. Vector, Monobook) and currently hardcoded, the module may need to be altered to allow variation through the skinStyles feature. This way Minerva can provide its own version of those styles.
  • Move to Skin classs.

    If the feature in question can't be done in a generic way, and should not be required to exist in every skin, we need to make it the Skins's responsibility to load the module. This way Minerva, as a skin, can just straight-forward decide not to load it or to load something else instead. In case of mediawiki.toc we probably want to go this route. Instead of having the Parser or OutputPage load it, expose the boolean presence of a TOC and let the Skin be in charge of loading a module if and however it wants to.

@Jdlrobson I agree that we've got modules loaded unconditionally right now that shouldn't execute on mobile. I'm not suggesting we remove targets without solving that first. (That'd actively break things indeed.)

There is a sizeable amount of technical debt hiding behind this that we've been ignoring. We're loading modules that we shouldn't be loading. Right now the "targets" system is intercepting this by pretending certain modules don't exist and ignoring their load requirement.

This is counter-intuitive. When writing code that adds a module to a page, the code rightfully assumes that it will be loaded. It magically disappearing creates an unexpected relationship between core, random extensions and MobileFrontend's potential replacement of such module.

As such, various special pages are currently broken on mobile. (Though, fair enough, some of them would be equally broken if we did load the modules as they weren't written for mobile.) For special pages we don't want to invest in for mobile yet, I suggest we just let them be. Presumably MobileFrontend already makes them undiscoverable. It'd mostly be people opening them directly, in which case we should let them. Especially as our code and browsers improve over time. (If really important, MobileFrontend could remove or override the page using $wgSpecialPages and display a helpful message of sorts.)

I suggest the following steps:

  1. Emit a run-time warning if a module is added to OutputPage that isn't legal for the current request's target.
  2. Address those warnings.
  3. Remove the target system.

Yep this sounds like a really good workflow.
For step 1 T137772 will probably suffice?
Great write up. Thank for you getting your thoughts down.

To be clear @Krinkle I consider T137772 a blocker and this should not be attempted without that.

To be clear @Krinkle I consider T137772 a blocker and this should not be attempted without that.

  • This task (and T140675) add server-side logging when modules are added outside their target. Once done, and dealt with all cases (per T127268#2463941) we'll remove the then-noop targets system. This will probably take months to complete because many cases require larger refactoring.
  • T137772 is about adding a minor feature to allow declaratively deprecate individual modules, and log (client-side?) when they are loaded.

Both tasks involve "deprecation" and "logging" of some kind, but otherwise they're unrelated?

I see what you mean. I guess T137772 will help inform T140675 but yes provided T140675 is a blocker for this task that should be enough.

It's worth pointing out that dismantling targets would result in Mustache and Hogan template compilers co-existing in certain places. This is currently worked around in the Cards extension by a Muhgan compiler which uses Hogan/Mustache compatible templates with whichever compiler is available https://github.com/wikimedia/mediawiki-extensions-Cards/blob/master/includes/ResourceLoaderMuhoganModule.php

Something we should think through before dismantling.

It's worth pointing out that dismantling targets would result in Mustache and Hogan template compilers co-existing in certain places.

Why do these register the same template engine name? Are they fully interchangeable? If so, why do have Hogan still?

The mobile site never migrated to Mustache due to a lack of motivation for 2 reasons (outweighing the benefit so far):

  1. it would require a review and rewrite of all our templates. It seems mustache does a few things differently. For instance {{#foo}}bar{{/foo}} in Hogan doesn't render bar if the template variable foo is null or empty string. In Mustache it renders only if foo is a boolean and true.
  2. Mustache is still substantially bigger than Hogan ( 3.4k).

The template can be fully interchangeable but only if written in a certain way. This extension had the unique problem of needing to ship templates to both mobile and desktop and we didn't want to ship both Hogan and Mustache to end users unnecessarily.

Krinkle added a comment.EditedAug 16 2016, 11:36 PM

It's worth pointing out that dismantling targets would result in Mustache and Hogan template compilers co-existing in certain places. This is currently worked around [by] a Muhgan compiler which uses Hogan/Mustache compatible templates with whichever compiler is available https://github.com/wikimedia/mediawiki-extensions-Cards/blob/master/includes/ResourceLoaderMuhoganModule.php

	public function getDependencies( ResourceLoaderContext $context = null ) {
		$dependencies = parent::getDependencies( $context );
		if ( $context && $context->getRequest()->getVal( 'target' ) === 'mobile' ) {
			$dependencies[] = 'mediawiki.template.hogan';
		} else {
			$dependencies[] = 'mediawiki.template.mustache';
		}

This only works by accident. The target query parameter exists if-and-only-if the request is for the startup module (in which the dependencies are registered). Unlike what one might expect, target was never added to ResourceLoaderContext, so this can cause cache pollution in its current form.

We can find any of numerous ways to override Mustache with Hogan on mobile. For example, by having hogan.js call registerCompiler with mustache instead of or in addition to hogan. Or, while not preferred, we could allow overriding the declarative module loading based on template name with an explicit server-side registry. (See https://gerrit.wikimedia.org/r/#/c/180647/ for past discussion).

Either way, the bottom line is, targets does not solve the problem. Right now, MediaWiki core isn't using mustache templates client-side yet. But once it does (which I think we both want) we'll have a situation where, as usual, targets will break it on mobile. The core script will depend on mediawiki.templates.mustache which will be missing on mobile, and fail in non-graceful ways for the end-user.

The targets system is preventing it from loading the dependency, but it's not enabling either core or MobileFrontend to allow use of Mustache or Hogan in a way that works on desktop and mobile. It rather encourages more duplication and isolation by requiring mobile to reject all core JS and re-implement anything that uses templates.

Alternatively, we could discuss more generally whether we want to use Mustache.js or Hogan.js - regardless of desktop or mobile.

In the Front-end standards group we settled on Mustache.js. While I don't remember the conversations, I think the expectation was for MobileFrontend to migrate its use of Hogan.js. I'm happy for us to reconsider this decision. From a quick look, it seems Twitter's Hogan.js is conceptually in better shape. Though I think the performance gain has mostly levelled out since Mustache.js is actively developed while Hogan stopped being maintained in 2014 (with one unreleased fix in 2015).

@Krinkle agree with all you write here. It's something we have to resolve in the future. The reason MobileFrontend never migrated away from hogan.js was purely down to priorities which will change as you make progress with this work.

The solution is clearly to share one templating solution across mobile and desktop. Whether that should be Mustache or Hogan I must confess I haven't had time to think about that too much. The mobile site can definitely make use of Mustache but we have to spend some time checking/migrating all the templates and testing the crap out of it to allow that. We can definitely discuss in frontend standards if necessary. I've sent a mail to the reading team to think about this.

This proposal is selected for the Developer-Wishlist voting round and will be added to a MediaWiki page very soon. To the subscribers, or proposer of this task: please help modify the task description: add a brief summary (10-12 lines) of the problem that this proposal raises, topics discussed in the comments, and a proposed solution (if there is any yet). Remember to add a header with a title "Description," to your content. Please do so before February 5th, 12:00 pm UTC.

(Patch is proof of concept so removing from review queue until we deal with the blockers)

brion added a subscriber: brion.Feb 15 2017, 12:50 PM

So the correct fix for this is to remove the "mobile site" mode and extra skin and have a single, responsive output mode and skin, with all modules working on that skin on both large and small screens. This is a fairly big task.

(Or I suppose you can keep two skins and force everyone to ACTUALLY test everything on both.)

brion added a comment.Feb 15 2017, 1:04 PM

Anyway, main thing to consider is that the targets system was meant to be temporary, as I recall, during the period when we had massive amounts of stuff that wasn't tested on mobile. If people still aren't testing things on mobile, then we got a problem that needs fixing before we remove it entirely. :)

So the correct fix for this is to remove the "mobile site" mode and extra skin and have a single, responsive output mode and skin, with all modules working on that skin on both large and small screens. This is a fairly big task.

FWIW i don't agree these are the same. I tend to lean towards your follow up comment. One of the benefits of having 2 separate sites is you can control experiences for instance have Page Previews that show on hover on a desktop site but not on a mobile site.

Fwiw people are not testing on the mobile skin so protection is helpful. Usually mobile is an afterthought and addressed by slapping some display nones on the page... We need to break out of that and think about things like touch areas... Avoiding clutter in interfaces etc. Im still not sure how we help people think like this.

That said a lot of code doesnt load on mobile simply because of targets... Most of the time this is a good thing but in some cases e.g. APISandbox this is accidental and leads to broken experience. i'd much rather we got to a stage where this was explicit not accidental. Timo's patch to add logging will help us deprecate this much quickly or at least get us to a point where the default targets are both.

brion added a comment.Feb 15 2017, 4:43 PM

@Jdlrobson agreed, I apologize for my hasty snarky comment earlier. :)

brion added a comment.Feb 15 2017, 4:49 PM

I've also opened a more general tech-debt task T158181 to collect subtasks where we have different or limited functionality between mobile and desktop web. We may not reach 1:1 parity but I'd love to try. :)

TheDJ added a subscriber: TheDJ.Feb 15 2017, 5:33 PM

Anyway, main thing to consider is that the targets system was meant to be temporary, as I recall, during the period when we had massive amounts of stuff that wasn't tested on mobile.

That was the primary reason yes, the secondary reason was to reduce overhead of unnecessary cruft (it was very important in a time where content could not yet add RL modules to a page output for instance). Reducing that overhead, might still be a valid use of the target system. As I said before, I can see a 'Lite' and/or Zero version of the website and maybe the target system could provide that. I don't know.

But I personally think this is a terribly worded ticket. It sounds a bit like "dismantle CSS media queries, because it introduces bugs if we don't design or test on media queries that don't match our resolution of 1024px". And a bad premise makes does not make for constructive path forward usually.

I think it's more: Decide if we want the overhead of having a targets system, potentially get rid of it. Blocked by our dependency on mobile and desktop targets in order to serve pages for mobile and desktop platforms, blocked by not having skins and content styling that is screen size and interaction method (finger controls vs mouse pointer) independent.

Agreed @TheDJ from the description people seem to be complaining that they have to write patches "to add target=>mobile to my resources".
TBH I interpret this as people didn't think to test on mobile until the last minute. We can of course make the default targets=>desktop,mobile to make this problem go away but would we want to do this if this encourages people not to think of mobile at all...?

TheDJ added a comment.EditedFeb 15 2017, 8:52 PM

TBH I interpret this as people didn't think to test on mobile until the last minute.

Which is logical, there are so many things that you have to consider for our complex eco system, that it's impossible to mentally keep track of all that.

I see it more as: People are annoyed that this new glass door has been installed (problem), which they keep running into when they want to leave the building (actual problem). They suggest removal of the door (as a solution). Alternatives (better solutions?) might be not shutting the door by default (have problems occur and then fix them later), installing an automatic sensor to open the door (jenkins linting or automated testcases) or sticking one of those bird stickers on it (git hook "did you run grunt test mobile ?"), write a step-by-step guide for exiting the building (requirements checklist for review).

but would we want to do this if this encourages people not to think of mobile at all...?

Possibly. We should remember that there is a whole lot more safeguards in place in mobile pageviews then initially. Effects are likely to be significantly less catastrophic then when we started from scratch. It would also make problems more visible, giving people more incentive to actually go out and fix them. By having everything in safe mode, there is no reason for people to consider the dangerous stuff. Perhaps safe mode should be the fallback strategy, and not the default strategy.

Which is logical, there are so many things that you have to consider for our complex eco system, that it's impossible to mentally keep track of all that.

I wouldn't say it's impossible but it is a choice made by the engineer.

I see it more as: People are annoyed that this new glass door has been installed (problem), which they keep running into when they want to leave the building (actual problem). They suggest removal of the door (as a solution). Alternatives (better solutions?) might be not shutting the door by default (have problems occur and then fix them later), installing an automatic sensor to open the door (jenkins linting or automated testcases) or sticking one of those bird stickers on it (git hook "did you run grunt test mobile ?"), write a step-by-step guide for exiting the building (requirements checklist for review).

Agreed. There are definitely other approaches to solving this problem.

Tgr added a subscriber: Tgr.Feb 15 2017, 11:38 PM

The fundamental difference between a separate mobile site and a responsive skin is that with the first it is easier to control the response size. Given our strategic focus on the Global South, that seems pretty important.

Restricted Application added a project: Performance-Team. · View Herald TranscriptFeb 22 2018, 10:19 PM

Change 421204 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Document 'target' query param in StartupModule

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

Change 421204 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Document 'target' query param in StartupModule

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

Change 431816 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Bump severity of targets violation from DEBUG to INFO

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

Jdlrobson updated the task description. (Show Details)May 8 2018, 7:05 PM

Change 431816 abandoned by Krinkle:
resourceloader: Bump severity of targets violation from DEBUG to INFO

Reason:
To be re-opened later.

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