= 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
[x] 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 ?)
[] Pass instance of Toggler to Skin.js and make Skin subscribe to Toggler's events
[] Pass instance of Skin to InfiniteScroll, ImageOverlay and PointerOverlay (this may involve passing Skin to all Overlay instances via OverlayManager)
[] 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 [[ https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/477682/ | Remove usage of global event emitter from Nearby ]]
MF [[ https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/477683/ | Remove usage of global event emitter from Talk Overlays ]] and [[ https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/477688/ | Minerva Remove usage of global event emitter from skins.minerva.talk/init.js ]]
MF [[ https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/477684/ | Remove usage of global event emitter from MF ]] and Minerva [[ https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/477689/ | Remove usage of global event emitter from Minerva ]]
MF [[ https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/477685/ | 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