Page MenuHomePhabricator

Merge bootstrap and render code in RelatedArticles
Open, LowPublic3 Story Points

Description

When discussing usages of mw.track inside RelatedArticles and whether it should use mw.hook, we came to the conclusion that we didn't need to use either.

The use of the hook/track dates back to now obsolete requirements that related articles rendered differently under different circumstances.

Since this is not the case, we believe this code can be simplified by merging the render and bootstrap code into one single file and module.

We will load all code on page load - it's not worth the complexity and fragmentation of cache to defer load the related articles rendering code given its size and the fact it will be cached after the initial page view.

Given the discussion in T176211, relating to serving unnecessary JavaScript we should

  • divide the code into 2 modules - a lightweight ext.relatedArticles.readMore.bootstrap module and a larger ext.relatedArticles.lib
  • mw.trackSubscribe( 'ext.relatedArticles.init' inside ext.relatedArticles.readMore should become an init method that is exported by the module
  • init should be invoked inside the loadRelatedArticles method of ext.relatedArticles.readMore.bootstrap

Background conversation

See discussion at T143572: Why are we using mw.track in related pages as an event bus?
Much of this is redundant, but explains the initial motivation for this card.

For consistency sake and good usage of the apis (regarding the docs) usages of mw.track (for example in RelatedArticles) that are not analytics related should be changed to use mw.hook instead.

Previous convos

IIRC I chose to use mw.track and mw.trackSubscribe in this way because I wanted to:

  1. keep the data source and the renderer (specific to Vector, Minerva, etc.) cleanly separated;
  2. avoid writing code like ext.relatedArticles.render = /* ... */; and
  3. ensure that the event was received by the listener, regardless of when the listener was loaded

@phuedx I agree with why you used it, but I'm not sure mw.track is the best option given that the docs state it is for analytics purposes.
I believe there's already a global event bus in the form of mw.hook that would probably be more appropiate:

mw.hook

This framework helps streamlining the timing of when these other code paths fire their plugins (instead of using document-ready, which can and should be limited to firing only once).

VS

mw.track

Track an analytic event.
This method provides a generic means for MediaWiki JavaScript code to capture state information for analysis.

At the end they are both as useful for this specific use case, I just wondered about the approach because of what the docs say.

Acceptance criteria

  • logReady, logEnabled, init should use mw.hook not mw.track

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 27 2016, 3:48 PM
ovasileva triaged this task as Normal priority.Nov 2 2016, 7:51 PM
ovasileva moved this task from To Triage to Triaged but Future on the Readers-Web-Backlog board.
Jdlrobson lowered the priority of this task from Normal to Low.Apr 17 2017, 10:52 PM
Jdlrobson added a project: patch-welcome.
Jdlrobson added a subscriber: Jdlrobson.

De-prioritising related pages work.

@Jhernandez Note that there is one important difference between mw.track and mw.hook. While both fire retroactively when subscribing after an event has already fired, where mw.track will apply all previous triggers, mw.hook only remembers the last one (or "current one").

mw.track('foo', 1);
mw.track('foo', 2);
mw.trackSubscribe('foo', console.log.bind(console));
// foo 1
// foo 2
mw.track('foo', 3);
// foo 3

mw.hook('bar').fire(1);
mw.hook('bar').fire(2);
mw.hook('bar').add(console.log.bind(console, 'bar'));
// bar 2
mw.hook('bar').fire(3);
// bar 3

The intent for mw.track is to track things that have happened from historical perspective (e.g. things typically forwarded to EventLogging or statsd, like triggers of JavaScript deprecations, load times, and others).

The main purpose of mw.hook is customisation and communication between active code paths that may interact. In the example case of wikipage.content, after a user has saved an edit with VisualEditor, or performed a preview with Live Preview (wikitext editor), any freshly loaded gadget that hooks into wikipage.content need not first apply to the old (now invisible) content. In addition, we wouldn't want to hold on to all that in memory, either, so that it may be garbage collected.

Thanks for the clarification @Krinkle. It fits well the use case since this task refers to an event triggered when scrolling (just once) through the viewport to initialize the lazily loaded related articles.

I'll add some more detail on the description.

Jhernandez updated the task description. (Show Details)Jun 12 2017, 9:26 AM
Jdlrobson raised the priority of this task from Low to Normal.Jun 28 2017, 6:49 PM

bumping visibility

Jdlrobson renamed this task from Change usage of `mw.track` to `mw.hook` when not used for analytics to Merge bootstrap and render code in RelatedArticles.Aug 15 2017, 4:49 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson set the point value for this task to 3.

Given T176211 should we reconsider this?

Jdlrobson lowered the priority of this task from Normal to Low.Nov 2 2017, 5:28 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task from Needs Analysis to Triaged but Future on the Readers-Web-Backlog board.

Updated description. We can talk about this plan later if and when we get round to working on it.

Krinkle removed a subscriber: Krinkle.
Jdlrobson raised the priority of this task from Low to Normal.Oct 3 2018, 8:02 PM

Bumping visibility

Jdlrobson lowered the priority of this task from Normal to Low.Apr 4 2019, 9:41 PM

Reverting back to low. Making it normal didn't help get it prioritised so clearly we haven't got bandwidth to work on this right now. Volunteers still appreciated!