Page MenuHomePhabricator

Merge bootstrap and render code in RelatedArticles
Closed, ResolvedPublic3 Estimated 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](https://doc.wikimedia.org/mediawiki-core/master/js/#!/api/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

ovasileva triaged this task as Medium priority.Nov 2 2016, 7:51 PM
ovasileva moved this task from Incoming to Triaged but Future on the Web-Team-Backlog board.
Jdlrobson lowered the priority of this task from Medium to Low.Apr 17 2017, 10:52 PM
Jdlrobson added a project: patch-welcome.
Jdlrobson subscribed.

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.

Jdlrobson raised the priority of this task from Low to Medium.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.
Jdlrobson lowered the priority of this task from Medium to Low.Nov 2 2017, 5:28 PM
Jdlrobson updated the task description. (Show Details)

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

Jdlrobson raised the priority of this task from Low to Medium.Oct 3 2018, 8:02 PM

Bumping visibility

Jdlrobson lowered the priority of this task from Medium 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!

This is a somewhat involved good first task. It's a well-isolated task that would require a deep-ish dive into how we try to design our modules for front-end performance.

Jdlrobson moved this task from Triaged but Future to unsed on the Web-Team-Backlog board.
Jdlrobson added a subscriber: ovasileva.

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

[mediawiki/extensions/RelatedArticles@master] Simplify the RelatedArticles extension to use WVUI

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

Test wiki created on Patch demo by Jdlrobson using patch(es) linked to this task:

https://patchdemo.wmflabs.org/wikis/30047fa6f5/w/

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

[mediawiki/extensions/RelatedArticles@master] Simplify the RelatedArticles extension to use Codex

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

Change 705033 abandoned by Jdlrobson:

[mediawiki/extensions/RelatedArticles@master] Simplify the RelatedArticles extension to use WVUI

Reason:

Rewritten in Codex: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/RelatedArticles/+/769143

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

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

[mediawiki/extensions/RelatedArticles@master] RelatedArticles should not use mw.trackSubscribe

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

Change 782256 merged by jenkins-bot:

[mediawiki/extensions/RelatedArticles@master] RelatedArticles should not use mw.trackSubscribe

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

Test wiki on Patch demo by Jdlrobson using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/30047fa6f5/w/