Page MenuHomePhabricator

[Spike, 8hrs] Discuss OO.EventEmitter usage for Views
Closed, ResolvedPublic0 Story Points

Description

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?

NOTE: the original task below is preserve for posterity. It adds no requirements to the above task but may increase context.

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/

Sign off steps

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application changed the subtype of this task from "Deadline" to "Task". · View Herald TranscriptDec 11 2018, 9:28 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

I'm wondering if we should go a step further and get rid of OO.EventEmitter altogether - instead making use of explicit properties bounded to the current context e.g.

function ExampleView(options) {
  this.onScroll = options.onScroll.bind(this);
}

var view = new ExampleView( { onScroll: doThis } );

This would be very [P]react compatible...

Following up on T211724#4815962, it looks like before-section-toggled, and talk-added-wo-overlay could all be props, and might be a good first step in this direction
e.g.

TalkSectionAddOverlay becomes:

diff --git a/resources/mobile.talk.overlays/TalkSectionAddOverlay.js b/resources/mobile.talk.overlays/TalkSectionAddOverlay.js
index 466c4e966..b97b0a8e8 100644
--- a/resources/mobile.talk.overlays/TalkSectionAddOverlay.js
+++ b/resources/mobile.talk.overlays/TalkSectionAddOverlay.js
@@ -138,9 +138,10 @@
 			this.showHidden( '.saving-header' );
 			this.save().then( function ( status ) {
 				if ( status === 'ok' ) {
-					if ( isOnTalkPage ) {
-						self.eventBus.emit( 'talk-added-wo-overlay' );
-					} else {
+					if ( self.options.onSaveComplete ) {
+						self.options.onSaveComplete();
+					}
+					if ( !isOnTalkPage ) {
 						self.pageGateway.invalidatePage( self.title );
 						toast.show( mw.msg( 'mobile-frontend-talk-topic-feedback' ) );
 						self.eventBus.emit( 'talk-discussion-added' );

and:

diff --git a/resources/skins.minerva.talk/init.js b/resources/skins.minerva.talk/init.js
index 0cf6bae251..fd4a700532 100644
--- a/resources/skins.minerva.talk/init.js
+++ b/resources/skins.minerva.talk/init.js
@@ -1,5 +1,6 @@
 ( function ( M, EventEmitter ) {
-	var loader = M.require( 'mobile.startup/rlModuleLoader' ),
+	var onSaveComplete,
+		loader = M.require( 'mobile.startup/rlModuleLoader' ),
 		LoadingOverlay = M.require( 'mobile.startup/LoadingOverlay' ),
 		eventBus = new EventEmitter(),
 		$talk = $( '.talk' ),
@@ -38,6 +39,7 @@
 
 	overlayManager.add( /^\/talk\/?(.*)$/, function ( id ) {
 		var talkOptions = {
+			onSaveComplete: onSaveComplete,
 			api: new mw.Api(),
 			title: title,
 			// T184273 using `getCurrentPage` because 'wgPageName' contains underscores instead of
@@ -80,7 +82,7 @@
 
 	if ( inTalkNamespace ) {
 		// reload the page after the new discussion was added
-		eventBus.on( 'talk-added-wo-overlay', function () {
+		onSaveComplete = function () {
 			var loadingOverlay = new LoadingOverlay();
 
 			window.location.hash = '';
@@ -90,6 +92,6 @@
 				loadingOverlay.show();
 				window.location.reload();
 			}, 10 );
-		} );
+		};
 	}
 }( mw.mobileFrontend, OO.EventEmitter ) );
nray added a comment.Dec 12 2018, 9:08 PM

@Jdlrobson I really like the more granular pattern you've suggested which, as you said, is similar to how react does it and think it could work really well. We should talk about it at the next super-happy-dev-time. The thing that I didn't like about my approach is that it made each component responsible for knowing about an event bus and how to use it. Your suggestion would resolve that by moving that layer out of components (which I think is a good thing). By doing this, it seems like it would make our components more like presentational components and less like container + presentational components.

Regarding getting rid of our OO.EventEmitter usage altogether, how do you propose we handle changes that multiple components need to know about? e.g. we have multiple components that listen to the "scroll:throttled" event from mobile.init/init.js which I think would be difficult to achieve without some sort of pub/sub model.

Added to agenda!

Jdlrobson renamed this task from Remove OO.EventEmitter functionality from View.js to [Spike] Discuss OO.EventEmitter usage for Views.Dec 12 2018, 10:48 PM
Jdlrobson renamed this task from [Spike] Discuss OO.EventEmitter usage for Views to [Spike, 8hrs] Discuss OO.EventEmitter usage for Views.Dec 18 2018, 12:37 AM
Jdlrobson added a project: Spike.
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task from Needs Prioritization to Upcoming on the Readers-Web-Backlog board.

We chatted about this today and here's a few highlights:

  • nick's patch was enlightening to all of us
  • fixing this is likely to happen on a case by case basis
  • we also use hooks to manage integrations with other services
  • local eventBus's could be substituted with callbacks
  • we might want to consider using hooks for global events to share between extensions but we prefer localisation where possible

remove OO.EventEmitter from View.

  • We can't just remove the OO.EventEmitter mixin as certain classes e.g. Drawer and CtaDrawer use this.on
  • Removing usages of this.on might make code more readable.

Chatting we felt like a next good step would be a spike. I've repurposed this task as such and am moving to estimation. @nray @phuedx @Jdrewniak @Niedzielski @pmiazga let me know if anything is not quite right about the scope of this spike!

nray updated the task description. (Show Details)Dec 18 2018, 8:37 PM
nray updated the task description. (Show Details)Dec 18 2018, 8:54 PM
nray awarded a token.Dec 18 2018, 9:13 PM
Niedzielski updated the task description. (Show Details)Dec 18 2018, 9:28 PM
Jdlrobson raised the priority of this task from Normal to Needs Triage.Jan 3 2019, 10:38 PM
Jdlrobson triaged this task as Normal priority.Jan 7 2019, 6:13 PM

Change 482878 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: extract lazy image loading from Skin

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

Change 482885 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] Hygiene: revise lazy image loading API

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

Change 483010 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: remove old lazy load image API

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

Change 483515 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: reduce jQuery in lazyImageLoader

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

Change 483601 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: consolidate section-toggled events

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

Change 484305 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: move lazy image loader placholder query

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

Change 484306 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] Update: use lazyImageLoader to query placeholders

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

Change 484576 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: remove Skin "changed" event

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

Change 484748 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: isolate lazy image load state

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

Change 484767 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: separate lazy references loading

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

Change 482878 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: extract lazy image loading from Skin

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

Change 483516 had a related patch set uploaded (by Jdlrobson; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] Hygiene: revise lazyImageLoader.loadImages() API

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

Change 482885 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Hygiene: revise lazy image loading API

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

Change 485122 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: make lazyImageLoader jQuery usage consistent

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

Change 485956 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] POC: Hygiene: replace section-toggled events

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

Change 485957 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] POC: Hygiene: replace section-toggled events

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

Change 483010 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: remove old lazy load image API

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

Sorry Stephen this is slipping down my priorities due to AMC and finishing up the webpack work plus the usual interrupt work. Will reach out once I'm on top of other things to help out with the dependency order.

Change 486372 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: add tests to lazyImageLoader

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

Change 485956 abandoned by Niedzielski:
POC: Hygiene: replace section-toggled events

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

Change 485957 abandoned by Niedzielski:
POC: Hygiene: replace section-toggled events

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

Change 483516 had a related patch set uploaded (by Jdlrobson; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] Hygiene: revise lazyImageLoader.loadImages() API

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

Change 483601 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Hygiene: consolidate section-toggled events

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

Change 484576 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: remove Skin "changed" event

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

Change 484748 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: isolate lazy image load state

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

Change 484767 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: separate lazy references loading

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

Change 485122 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: make lazyImageLoader jQuery usage consistent

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

So it's my understanding that https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/485957/ and https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/485956/ are the outcomes of this spike and we're just waiting a write up/discussion of that experience. Is that correct?

Yes, the offsite would be a good venue for this.

Change 484576 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: remove Skin "changed" event

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

Change 484748 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: isolate lazy image load state

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

Change 484767 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: separate lazy references loading

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

Change 485122 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: make lazyImageLoader jQuery usage consistent

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

Added discuss bullet to next super happy dev time.

Niedzielski removed Niedzielski as the assignee of this task.Feb 11 2019, 6:57 PM

We discussed this in super happy dev time. Removing myself as assignee.

Following up from yesterdays discussion, one thing we talked about is when it's appropriate to use and event bus and when it's not.

In general, an event bus is best used to create loose coupling between components that have distinct responsibilities (best example is event-logging, where one component is mainly responsible for analytics, while the other is responsible for UI, but we want them to talk to each other, sometimes).

In MF and Minerva however, we sometimes use events to call methods within the same class or subclass (i.e. within the same inheritance chain).

An example of this is how Panel.js emits a show/hide event self.emit( 'show' ); but this event is actually only bound in it's child class of Drawer.js. This pattern makes it difficult to understand where a method is actually executed, because not only do you have to understand the whole inheritance chain, you also need to identify where the method is bound and where in the chain the events are being triggering as well.

I think this kind of behaviour could be replaced with calling class methods directly instead of through an event bus.

nray claimed this task.Feb 12 2019, 6:05 PM
nray added a comment.Feb 12 2019, 9:07 PM

Following up from yesterdays discussion, one thing we talked about is when it's appropriate to use and event bus and when it's not.
In general, an event bus is best used to create loose coupling between components that have distinct responsibilities (best example is event-logging, where one component is mainly responsible for analytics, while the other is responsible for UI, but we want them to talk to each other, sometimes).
In MF and Minerva however, we sometimes use events to call methods within the same class or subclass (i.e. within the same inheritance chain).
An example of this is how Panel.js emits a show/hide event self.emit( 'show' ); but this event is actually only bound in it's child class of Drawer.js. This pattern makes it difficult to understand where a method is actually executed, because not only do you have to understand the whole inheritance chain, you also need to identify where the method is bound and where in the chain the events are being triggering as well.
I think this kind of behaviour could be replaced with calling class methods directly instead of through an event bus.

Additionally, we also discussed that it may be important to focus first on refactoring our components and ensuring their responsibilities are well defined and separated before we tackle switching out event bus usages with callbacks. It was noted that classes like Skin.js which has many different responsibilities added difficulty to this process because it was hard to isolate code to a point where callbacks could easily be passed.

@Jdlrobson @Niedzielski please let me know if I've missed anything. The sign-off step instructs to take off the spike tag and move this ticket to the backlog, but I'm not sure what this ticket should be retitled as it sounds like this ticket has now turned into an epic covered by T195482.

Jdlrobson closed this task as Resolved.Feb 12 2019, 11:06 PM

I've added an acceptance criteria to T195482 to make sure we follow up on this. Thanks @nray and @Jdrewniak for your detailed notes and of course to @Niedzielski for the research!