Routing events between handlers in the JS framework

[NateWr]

This issue is a placeholder to track some work/experimentation to improve the way that our JS handlers can communicate between themselves. The suggested approach is to have a kind of global event router which is based on the observer pattern common in several JS frameworks.

I'd like to use this to document some of the lessons learned about how events in the JS Handlers (eg - UI "widgets) work.

Overview

As a general rule, events that are executed on a Handler do not bubble up like normal JS events. They're captured in an effort to ensure that Handlers remain encapsulated.

However, in some cases we have needed Handlers to talk to each other. This is done by white-listing certain events to be "published", using the publishEvent, trigger and triggerPublicEvent_ methods. These published events are triggered on the Handler's parent element in the DOM, from which they can bubble up.

This event is captured by a parent DOM element which has a handler attached, typically a complex Handler like a grid or the SiteHandler.

Because modals exist outside the normal DOM hierarchy, bubbling doesn't really get the job done. To accomodate this, there's a eventBridge option which can be passed to Handlers. This is used by modals to redirect events to the LinkAction which opened the modal.

The notifyUser event also gets it's own routing. Several handlers deliberately bubble the event up to the SiteHandler, which then makes a request to the server to decide what notifications to display.

So we have two ways for Handlers which are separate in the DOM (ie - not nested) to communicate with each other: an event that bubbles to SiteHandler and is then re-routed by SiteHandler, or an event bridge that reaches out to another element with a jQuery selector.

The maintenance problem

This architecture has worked to keep the Handlers highly re-usable. But it means that in cases where we do need Handlers to interact, including in cases where bubbling may not be sufficient, we have written a lot of code to reach out from one Handler to another.

The result has been that the connections between coupled Handlers tend to be ad-hoc, written into whatever method of the Handler happens to be doing that job right then. As a result, it's hard to have a clear sense of the dependencies between Handlers where they do exist, and to guard against breaking these when refactoring.

The (first) proposed solution

I looked into addressing this with the observer pattern, something I was familiar with from the Backbone.js framework. It would also give us an opportunity to start piecemeal refactoring towards a more common approach to JS-heavy applications.

The blocking problem with implementing the observer pattern

The observer pattern can’t be implemented because we lack a central registry of handlers. A handler which updates its content will remove large chunks of the DOM without tracking which handlers were caught up in the destruction. We will have references to listeners that are no longer present in the DOM (the lapsed listener problem).

The usual way to resolve this is with a central registry of handlers which deregister their events when removed (the dispose pattern). But because a handler doesn’t know what handlers are caught up in it’s refresh operations, it can’t trigger deregistration on embedded handlers (eg - a LinkActionHandler that's part of a GridHandler).

A new proposal

Without a significant refactor, we can’t have truly decoupled handlers. Instead, I’d suggest using a modified observer pattern in which the relationships between events and listeners are stored explicitly in the observer’s code. So handlers would not subscribe to events on the observer. Instead, the observer would have explicit instructions for passing specific events to specific handlers.
The typical observer pattern looks like this:

Sender.init() {
	Observer.trigger(‘changeEvent’);
}
Listener.init() {
	Observer.listenTo(‘changeEvent’, Listener.respondToChangeEvent);
}

The Observer can be completely ignorant of the events that are passing through it. The modified observer pattern I’m proposing would nest lookup logic in the Observer.

Sender.init() {
	Observer.trigger(‘changeEvent’, eventData);
}
Listener.init() {
	this.bind(‘changeEvent’, Listener.respondToChangeEvent);
}
Observer.trigger(event, eventData) {
	if (event === ‘changeEvent’) {
		$(‘.listener’).trigger(‘changeEvent’, eventData);
	}
}

The Observer would not store references to handlers. Instead, it would look up any specific handlers by their associated DOM elements, performing a fresh lookup each time an event was triggered.
This would provide decoupling of a sort, though the coupling would still have to be specified in a central Observer. It’s only marginally better than the current practice of triggering an event from one handler on another handler’s DOM element directly.

But is it really an improvement?

Relying on DOM lookups is not something I feel good about, to be honest. All this pattern really does is restructure the hacky DOM-based lookups away from the Handler into a central location (example). I think it will help (a little) with the maintenance issue by exposing these relationships more clearly. But the same could be done in theory with the existing eventBridge technique.

I don't see a way around these DOM lookup approaches without a pretty significant refactor of our Handlers. I've been wracking my brain but I haven't come up with anything.

One very simple refactor that I have made which I think is genuinely useful is that, on the PHP side, the JSONMessage object now supports multiple events. So you can register more than one event to return in a JSON response. Example: when publishing an issue you can return the dataChanged event that the grid is programmed to respond to, as well as a issuePublished event that can be broadcast separately.

The global event router, combined with multiple events in a JSONMessage, could be a good pattern to adopt more broadly (triggering notifications, tasks, etc from the PHP side). But I'm not sure it gets us very far down the road I originally had in mind of an event-driven UI with stronger decoupling of Handlers.

Examples

pkp-lib: https://github.com/NateWr/pkp-lib/commits/i2163_event_router
ojs: https://github.com/NateWr/ojs/tree/i2163_event_router

[asmecher]

Thanks, @NateWr, good reading.

Do you remember when you first started working with the PKP JS code that you proposed a change to the handler lifecycle? IIRC you wanted static calling or something. Would that be a useful thing to revisit in mangling an observer pattern into our DOM usage?

[NateWr]

I dug up my old doc on that when I was looking into this. I'm not sure it would solve the problems we're facing in this case, although I think questions about how we initialize components will keep coming up if we start to introduce parts of an existing framework.

I've come up with an ad-hoc way to get a sort-of registry of handlers. Basically, we have every handler crawl up the DOM hierarchy and register itself with the first handler it finds. So each handler gets a reference to handlers attached to elements within the DOM.

This works because in the majority of cases we do full-handler refreshes all at once. So when refreshing, a handler can loop through it's child handlers and de-register event listeners.

There are a few cases where we do partial refreshes and I'm working on those at the moment. Some I'm refactoring away. Others (like replacing single grid elements) I'll probably write something special to handle those cases. But it's a pretty small group of cases (maybe half a dozen) which I've been able to identify.

I'm hoping this will be pretty reliable, though I'll have to see once it's in place if we end up with memory leaks.

[asmecher]

@NateWr, thanks, that sounds like a decent short-term plan.

[NateWr]

@kaschioudi I think I've worked out the test issues. Do you have time to code review the PRs in this comment? #2163 (comment)

[kaschioudi]

@NateWr Everything looks fine to me 👍

[NateWr]

🎉 Really happy to have this merged in. Ready to start build more API stuff. :)

[asmecher]

@NateWr and @kaschioudi, is this one ready to be closed now that the OMP warning fix has been merged?

[NateWr]

💥