Re: [WikimediaMobile] Declarative DOM event delegation in JS views

Hello, great work, thanks :) I think the good practices and conventions section should be in our Coding Conventions[1], what do you think? [1] https://www.mediawiki.org/wiki/Mobile_web/Coding_conventions Grüße / Kind regards, Florian Von: mobile-l-bounces(a)lists.wikimedia.org [mailto:mobile-l-bounces@lists.wikimedia.org] Im Auftrag von Joaquin Oltra Hernandez Gesendet: Mittwoch, 31. Dezember 2014 11:03 An: mobile-l Cc: mobile-tech Betreff: [WikimediaMobile] Declarative DOM event delegation in JS views I'm happy to announce that we've landed on Mantle and MobileFrontend a new way to specify DOM events on a javascript view that I think will make our views better in several ways. TLDR: * Backbone style declarative delegated DOM events handling is in Mantle's View class. Use it when possible. TOC: * Why. Current approach to binding events * A different proven approach, delegated declarative DOM events map * Examples of this in MobileFrontend * Good practices and conventions * Gotchas * Next steps Why. Current approach to binding events --------------------------------------- Currently on JS views, when we want to bind some events to the view, we bind them on the `postRender` method, like so: var CounterView = View.extend({ // ... postRender: function() { // View display logic this.$('.increment').click(function(ev) { // ... }) this.$('.reset').click(function(ev) { // ... }) }, // ... }) With the current approach, a few bad side effects follow: * Everytime the view renders, we bind events (multiple of them), this is inefficient, there are better ways. * The postRender method on views grows a lot in size, event handling declarations and method code is mixed with display logic. * The complexity of the view (arguably) increases, and with it the ability to read and modify the source confidently. A different proven approach, delegated declarative dom events map ----------------------------------------------------------------- Inspired and brought from Backbone.js, we have ported their dom events map, which brings a way of specifying event and selector, and a handler that will be bound to the view, declaratively on the view itself, the same way we specify defaults, for example. http://backbonejs.org/#View-extend http://backbonejs.org/#View-delegateEvents The previous example, with this approach with our views would be: var CounterView = View.extend({ // ... events: { 'click .increment': 'increment', 'click .reset': 'reset', }, postRender: function() { // View display logic }, increment: function() { this.counter++; }, reset: function() { this.counter=0; }, // ... }) On View, we can now specify a `events` entry, which is a map of `event selector`: handler Where event is any valid jQuery event, selector is any valid jQuery selector, and handler is either a function, or a string (name of a method on the view). With this approach: * Events are bound with delegate, effectively attaching one DOM listener on the view's root DOM element. (Read http://api.jquery.com/on/#direct-and-delegated-events) * To see which DOM events the view binds and reacts to, just need to find the events map of the view. * Event handling code (usually pieces of logic of the view) gets a name, and it's encapsulated on its own method giving us: * Reuse of such logic/functionality * Easier and better testability * Cleaner postRender methods Internally, this is just using jQuery to delegate the events and the selectors to specified handlers bound to the view, no black magic. Examples of this in MobileFrontend ---------------------------------- https://gerrit.wikimedia.org/r/#/c/180835/ https://gerrit.wikimedia.org/r/#/c/180836/ https://gerrit.wikimedia.org/r/#/c/180837/ Notice that the code is much less nested and comes back to View level. In some cases the postRender method becomes unnecessary, and is deleted, and the code that was on it, has now a name and is properly encapsulated on the view. Good practices and conventions ------------------------------ When creating event handlers, if they are doing DOM stuff, like preventDefault, or extracting data from the DOM node, manipulating the DOM, etc, you should name the handler `onEvent`, for example `onThanksClick` or `onHeaderToggle`. If possible, extract functionality that makes sense independently into methods with a good name and call them from the handlers as needed. When creating methods on a view, like the `increment` example above, that happen to be called when an event happens but make sense independently, I would argue that a method name `increment` makes more sense than a handler name `onIncrementClick`, but that is open to common sense and taste I guess. If I had to also extract what to increment from a DOM element, and then increment, then I would do both, for example: // ... events: { 'click .increment': 'onIncrementClick' }, onIncrementClick: function(ev) { var amount = parseFloat(this.$('.amount').val()); this.increment(amount); }, increment: function(amount) { this.counter += amount || 1; }, // ... Gotchas ------- Currently, a common practice is inside a event handler to access the DOM element that initiated the event doing `$(this)`. With the events map the handlers/methods are at view level and are consistently bound to the view's this, so how do we access the DOM element of the event? Handler methods get an jQuery.Event as a parameter, so: onIncrementClick: function(ev) { var $incButton = $(ev.target); // ... http://api.jquery.com/category/events/event-object/ Next steps ---------- If there is any doubts or concerns, please ask them. If you find going along the code you spot a View that could benefit of this approach, open a Phabricator task (prefixed with "Hygiene" and in the MobileWeb team) and take on it ifyou want. I think this gives us a good opportunity to get more view tests, and make the code cleaner, more understandable and approachable for both us and possible contributors (Backbone style views are known by everybody at this point).