We've identified our usage of OO.EventEmitter as being problematic as all our components can operate with event buses but barely any of them use it. We will begin by exploring the problem some more and fleshing out this task with actionables.
== Spike
**Question: What would our code look like if it didn't have an OO.EventEmitter?**
Outcomes: A throwaway patch with a write up explaining next steps.
Sign off steps for spike: Update this task with findings, drop the spike tag and move it to the backlog.
=== Spike contsxt
> ag "this\.on\(" resources/m* src/
shows the following usages of `this.on`
```
resources/mobile.notifications.overlay/NotificationsFilterOverlay.js
21: this.on( 'hide', function () {
resources/mobile.search/SearchOverlay.js
270: this.on( 'search-start', function ( searchData ) {
278: this.on( 'search-results', clearSearch );
src/mobile.startup/Drawer.js
74: this.on( 'show', this.onShowDrawer.bind( this ) );
75: this.on( 'hide', this.onHideDrawer.bind( this ) );
src/mobile.startup/references/ReferencesDrawer.js
89: this.on( 'show', this.onShow.bind( this ) );
90: this.on( 'hide', this.onHide.bind( this ) );
src/mobile.startup/Skin.js
206: this.on( 'changed', _loadImages );
```
and the following usages of `this.emit`:
> ag "this\.emit\(" resources/m* src/
```
resources/mobile.mediaViewer/ImageOverlay.js
104: this.emit( ImageOverlay.EVENT_SLIDE, nextThumbnail );
308: this.emit( ImageOverlay.EVENT_EXIT, ev );
resources/mobile.mediaViewer/LoadErrorMessage.js
75: this.emit( 'retry' );
resources/mobile.scrollEndEventEmitter/ScrollEndEventEmitter.js
113: this.emit( ScrollEndEventEmitter.EVENT_SCROLL_END );
resources/mobile.search/SearchOverlay.js
226: this.emit( 'search-result-click', {
315: this.emit( 'search-show' );
src/mobile.startup/Overlay.js
182: this.emit( Overlay.EVENT_EXIT );
288: this.emit( 'hide' );
src/mobile.startup/Skin.js
124: this.emit( 'changed' );
```
All of these should be rewritten to not use the OO.EventEmitter. There will also be additional rewrites needed for usages of `self.on`/`self.emit` as well as for any component that listens to another component's `this.emit`/`self.emit` events (e.g. `this.on`/`self.on` only accounts for components that listen to their own events). What is nice about this approach? What is less nice?
== Original task description
While working on T156186, it was observed that View.js mixes in OO.EventEmitter:
```
function View() {
this.initialize.apply( this, arguments );
}
OO.mixinClass( View, OO.EventEmitter );
```
Since most of our components extend from View, they have the ability to act as an event bus whether they utilize this functionality or not. This brings numerous disadvantages including:
- Most of our unit tests will need to bring in the OO dependency even if the component under test doesn't emit or listen to events (e.g. Anchor.test.js, icons.test.js, Button.test.js)
- Unclear expectations on what to test: Should we test whether each component can successfully listen to or emit events?
- Difficult to reason about a given component's responsibilities
To date, the following files make use of the `this.emit` or `self.emit` and may be affected by this change (Note: this list does NOT include files that listen to these events; further investigation is needed to determine what is consuming these events):
```
core ᚠ master % Rg 'self\.emit\(|this\.emit\(' extensions/MobileFrontend skins/MinervaNeue
extensions/MobileFrontend/resources/mobile.scrollEndEventEmitter/ScrollEndEventEmitter.js
113: this.emit( ScrollEndEventEmitter.EVENT_SCROLL_END );
extensions/MobileFrontend/resources/mobile.search/SearchOverlay.js
226: this.emit( 'search-result-click', {
315: this.emit( 'search-show' );
369: self.emit( 'search-start', {
404: self.emit( 'search-results', {
extensions/MobileFrontend/resources/mobile.mediaViewer/LoadErrorMessage.js
75: this.emit( 'retry' );
extensions/MobileFrontend/resources/mobile.mediaViewer/ImageOverlay.js
104: this.emit( ImageOverlay.EVENT_SLIDE, nextThumbnail );
308: this.emit( ImageOverlay.EVENT_EXIT, ev );
extensions/MobileFrontend/src/mobile.startup/Overlay.js
181: this.emit( Overlay.EVENT_EXIT );
287: this.emit( 'hide' );
extensions/MobileFrontend/src/mobile.startup/Skin.js
124: this.emit( 'changed' );
357: self.emit( 'references-loaded', self.page );
extensions/MobileFrontend/src/mobile.startup/Panel.js
57: self.emit( 'show' );
73: self.emit( 'hide' );
skins/MinervaNeue/resources/skins.minerva.mainMenu/MainMenu.js
127: this.emit( 'open' );
extensions/MobileFrontend/tests/node-qunit/mobile.startup/OverlayManager.test.js
32: this.emit( 'hide' );
extensions/MobileFrontend/src/mobile.startup/watchstar/Watchstar.js
172: self.emit( 'watch' );
179: self.emit( 'unwatch' );
```
=== Proposal
- Determine a good way remove the reliance on View.js OO.EventEmitter functionality, but not break components that are depending on it. Some initial thoughts:
- Move the `OO.mixinClass( ..., OO.EventEmitter )` line only to the components that are currently utilizing that functionality. This seems like the lowest friction strategy (but not necessarily the best).
- Make components that currently emit local events call a callback instead that is injected into the constructor. Components that currently listen to local events would pass a callback to these constructors.
- It seems like some components emit and listen to their own events ( e.g. Skin.js and the 'changed' event ). Could this behavior be replaced with a function call instead?
- Inject a "localized" event bus into components that emit/listen . This might work well for components that are related to each other [1][2]
=== Acceptance Criteria
[] `OO.mixinClass( View, OO.EventEmitter );` line has been removed from View and components/code that used to depend on it still work
[1] https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/477683/
[2] https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/477682/