MFA: MobileFrontend should not have a global event emitter
Closed, ResolvedPublic5 Story Points

Description

Discussion to have

Various classes are tightly coupled with the global God M object.

For example Skin.js binds events to global scroll events.

In https://gerrit.wikimedia.org/r/333981 @Krinkle said:

Just advice: It may be useful to instead move away from it being global in the first place. Aside from DOM events (which should be bound on document or window directly), anything else surely can belong to an actual object instead of an anonymous global one?
For example, the events emitted by Skin, could be emitted _on_ the Skin itself (by mixing in EventEmitter). The only inconvenience that this introduces is that if there is an on() listener outside the skin module, that module must have a reference to the Skin object. if that is the case, that may in itself uncover debt that may be worth exploring first. If not, or if a reference to Skin is no problem, then making an emitter might be a better solution for you.

Existing usages

We expose the following global events (some throttled):

  • resize (emitted by Skin.js used by ImageOverlay)
  • scroll (emitted by Skin.js used by InfiniteScroll and skins.minerva.backtotop)
  • before-section-toggled (emitted by Toggler and used by Skin.js)
  • section-toggled (emitted by Toggler and used by Skin.js; e.g., lazily loading images)
  • talk-discussion-added (emitted by TalkSectionAddOverlay.js and used by TalkOverlay)
  • talk-added-wo-overlay (emitted by TalkSectionAddOverlay and subscribed to in skins.minerva.talk/init.js)
  • watched (emitted by WatchstarGateway used by nothing)
  • edit-preview (emitted inside EditorOverlay)
  • category-added (emitted by CategoryAddOverlay and used inside CategoryOverlay and resources/skins.minerva.options/categories.js)

Proposal

Goal: Stop mw.mobileFrontend from being an EventEmitter

  • Make TalkOverlay events talk-added-wo-overlay and talk-discussion-added local to the TalkOverlay
  • Remove unused events WatchstarGateway (watched), EditorOverlay (edit-preview) - https://gerrit.wikimedia.org/r/363658
  • Make event category-added local to component shared by both CategoryAddOverlay and CategoryOverlay (Page ?)
  • Toggler/Skin
  • ImageOverlay
  • Stop mw.mobileFrontend from being an EventEmitter

Code review session (3rd August)

We discussed how a global EventEmitter may not be a bad thing but how our existing usages may be inappropriate. We touched upon how we might want to use mw.hook instead for places it truly is needed.

Stephen: Very reasonable pattern for global events consumed by multiple parts. Things like configuration changes, etc.
Jon: Used in places for shortcuts, wild wild west
Sam: Likes event buses and decoupling producers and consumers. About M.on usages, the bus should be injected instead of used from the global. Makes more sense and testing better
Piotr: Why not mw.hook?
Jon: Implemented before that was in core
Sam: There is mw.track too
Joa: Mainly used as a shortcut to connect to parts of state. Avoids refactors and state ownership design work

QA

First, I am sorry. This card touched a lot of different files in MF and Minerva. Here are the patches that were merged for reference:

MF Remove usage of global event emitter from Nearby
MF Remove usage of global event emitter from Talk Overlays and Minerva Remove usage of global event emitter from skins.minerva.talk/init.js
MF Remove usage of global event emitter from MF and Minerva Remove usage of global event emitter from Minerva
MF Remove moduleLoader's event bus

The following were areas of MF and Minerva that were touched and thus might be good to check on the Mobile site:

  • Nearby
  • Talk Overlays
  • Category Overlays
  • Photo List
  • Image Overlay
  • Uploads
  • Toggle (section toggle, TOC toggle)
  • WatchList
  • Back to top scroll button
There are a very large number of changes, so older changes are hidden. Show Older Changes

In today's code review session we noticed how Skin.js subscribed to a 'resize' event inside MobileFrontend that is only provided in Minerva. This seemed like another red flag that the M.on approach is problematic.

Krinkle removed a subscriber: Krinkle.Sep 6 2017, 8:28 PM
Jdlrobson renamed this task from M should not be a global event emitter to MobileFrontend should not have a global event emitter.Feb 14 2018, 5:30 PM
Jdlrobson added a subscriber: Krinkle.
ovasileva set the point value for this task to 5.Feb 14 2018, 5:55 PM

Jon and I discussed this ticket briefly in passing the other day. A couple ideas that were mentioned: add an event-bus.js singleton with a couple functions, post() and subscribe(), and any events used by the codebase with @event JSDocs. e.g., EVENT_NEARBY_POST_RENDER, EVENT_NEARBY_ITEM_CLICK, etc.

Krinkle removed a subscriber: Krinkle.Mar 7 2018, 1:06 AM

I don't know if folks will like it but I started hanging event name constants off the prototype. e.g., https://gerrit.wikimedia.org/r/#/c/416884/2/resources/mobile.startup/Overlay.js

Change 428986 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: rename and symbolize InfiniteScroll event

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

Change 428986 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: rename and symbolize InfiniteScroll event

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

Change 440559 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Discourage use of global event emitter

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

Change 440559 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Discourage use of global event emitter

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

Krinkle updated the task description. (Show Details)Jul 6 2018, 11:11 PM
Krinkle added a subscriber: Krinkle.
Jdlrobson renamed this task from MobileFrontend should not have a global event emitter to MFA: MobileFrontend should not have a global event emitter.Thu, Nov 15, 6:52 PM

We discussed today that a possibly useful intermediate step might be to only separate the module loading and event emitter responsibilities into two files. It's not clear to me if this would be any easier than the scope of the original task but it would be an improvement over the current code.

Change 477422 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Remove usage of global event emitter

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

Change 477433 had a related patch set uploaded (by Nray; owner: Nick Ray):
[mediawiki/skins/MinervaNeue@master] Remove usage of global event emitter

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

Change 477682 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Remove usage of global event emitter from Nearby

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

Change 477683 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Remove usage of global event emitter from Talk Overlays

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

Change 477684 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Remove usage of global event emitter from MF

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

Change 477685 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Remove moduleLoader's event bus

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

Change 477422 abandoned by Nray:
Remove usage of global event emitter

Reason:
Abandoning in favor of smaller patches starting with I632124515d4c26ae5ce77dd503d00a62e5a65dda

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

Change 477688 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/MinervaNeue@master] Remove usage of global event emitter from skins.minerva.talk/init.js

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

Change 477689 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/MinervaNeue@master] Remove usage of global event emitter from Minerva

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

Change 477433 abandoned by Nray:
Remove usage of global event emitter

Reason:
Abandoned in favor of smaller patches starting with Ic766d0bbf2746df898038115e2e4bc791ac10359

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

nray removed nray as the assignee of this task.
nray added a subscriber: nray.
nray removed nray as the assignee of this task.
nray claimed this task.Thu, Dec 6, 11:54 PM

Change 478131 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Remove usage of global event emitter from Nearby

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

Change 478131 abandoned by Nray:
Remove usage of global event emitter from Nearby

Reason:
whoops somehow removed change id when rebasing. I632124515d4c26ae5ce77dd503d00a62e5a65dda is correct one

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

nray removed nray as the assignee of this task.Fri, Dec 7, 2:47 AM

Change 477682 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove usage of global event emitter from Nearby

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

Change 477683 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove usage of global event emitter from Talk Overlays

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

Change 477688 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Remove usage of global event emitter from skins.minerva.talk/init.js

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

Change 477684 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove usage of global event emitter from MF

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

Change 477689 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Remove usage of global event emitter from Minerva

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

nray removed nray as the assignee of this task.

Change 477685 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove moduleLoader's event bus

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

nray updated the task description. (Show Details)Tue, Dec 11, 7:07 PM
Jdlrobson updated the task description. (Show Details)Wed, Dec 12, 1:25 AM
Jdlrobson updated the task description. (Show Details)Wed, Dec 12, 1:39 AM

@nray can you respond to T211724#4815962 before I sign this off? Good job on this one! Nicely and carefully done!

QA is completed, and QA report below, but tldr is all works as expected but I found one new bug unrelated to this change (T211815)

QA:

Testing on https://en.m.wikipedia.beta.wmflabs.org/wiki/Barack_Obama

resize (emitted by Skin.js used by ImageOverlay)

  • Resize behaviour still working on mobile image overlay - height and width change as I resize.
  • Resize behaviour is NOT working on production OR beta cluster for the loading of skins.minerva.tablet.scripts (CREATE TASK)

scroll

(emitted by Skin.js used by InfiniteScroll and skins.minerva.backtotop)

  • Images are still loading as I scroll
  • Back to top hides and shows as I scroll

before-section-toggled

(emitted by Toggler and used by Skin.js)

  • References are still being loaded when I expand the reference section.

section-toggled

(emitted by Toggler and used by Skin.js; e.g., lazily loading images)

  • lazy loaded images handler fires when I toggle a section

talk-discussion-added

(emitted by TalkSectionAddOverlay.js and used by TalkOverlay)

  • When I add a talk topic the talk overlay refreshes.

talk-added-wo-overlay

(emitted by TalkSectionAddOverlay and subscribed to in skins.minerva.talk/init.js)
When I go to talk page, click "add discussion" and add a talk topic via the overlay on the talk page, the talk page itself refreshes.

watched

(emitted by WatchstarGateway used by nothing)

No event is emitted anywhere in the codebase!

edit-preview

(emitted inside EditorOverlay)

No event is emitted anywhere in the codebase!

category-added

(emitted by CategoryAddOverlay and used inside CategoryOverlay and resources/skins.minerva.options/categories.js)
Cannot be tested without T163121

Jdlrobson updated the task description. (Show Details)Wed, Dec 12, 10:43 PM
Jdlrobson closed this task as Resolved.

Signing this off