The future of the `events` hash

1

The events hash on Ember.Route is used for storing event handlers that can respond to .send or actions fired from templates via the {{action}} Handlebars helper. Presently, it’s only used within routes, and not within controllers, which means that controllers can handle .send or {{action}} events just by defining handlers right on the controller objects, while routes have to stash these handlers in events, which obviously inconsistent.

The events hash has had a limitation that it’s not easy to insert more event handlers in addition to ones inherited from a parent class or mixin without clobbering the whole events hash. A PR to address this and make event handlers inheritable (albeit presently without _super capabilities) has been submitted here, but this is reignited the conversation as to whether we really want/need the events hash.

We can either remove/deprecate the events hash from Ember.Route, or we can deprecate the definition of event handers directly on Controllers and Views. Known pros/cons thus far:

Remove events, define handlers directly on Route/Controller/View

  1. Changing this only affects Ember.Route
  2. Handlers would get inheritability and _super for free
  3. We will (without further effort) be preserving the possibility/likelihood that a send or {{action}} will unwittingly trigger some private API inherited from a parent class. A comically bad example of this is the fact that an {{action destroy}} fired from a template would be handled by your controller, effectively destroying the controller. Note that we can probably prevent a lot of these cases by more consistently underscore-izing private APIs, or coming up with some other solutions, but also keep in mind that this has been the default behavior for a while, and I haven’t heard much (if any) complaining about this.

Universalize the events hash, deprecate event handling directly on Controller/View

  1. Changing this would affect Ember.Controller / Ember.View
  2. Either clobber behavior is preserved, or we add sugar to allow for merging inherited events, and possibly (though way lower priority) support _super
  3. You could no longer treat a method as both an event and an internally (easily) callable method, e.g. today you can define a method like saveAll on a controller, and it can both handle saveAll actions from templates and can be called by anyone who has a reference to that controller, e.g. this.controllerFor('posts').saveAll(). Adding an events hash would require converting all your direct method calls to controller.send('someMethod'), and you’d also be sacrificing returns values, etc., and anything else that happens when you switch to an event-based approach.
  4. There would be no risk of accidentally running some unrelated inherited private API.

So…

How does everyone feel about this? We also have to keep in mind that we’re in RC and should try not to be TOO drastic, but I think we should tackle this inconsistency before 1.0. Thoughts?

3 Likes
2

I’m +1 on getting rid of the events hash because I found it to be unintuitive when I first started building a project in ember. I think the best course of action would be to add support for adding handlers on routes, with a deprecation warning for actions called through the events hash.

3

+1 on removing the events Hash, Routes can be extended and I end up in duplicating the handlers. (There is an alternate to it, but it’s out of the box).

Considering the steps involved in both the cases, steps in case 1 seems to be minimal.

4

+1 on removing events hash.

5

Also :+1: on deprecating the events hash, insane as the {{action destroy}} example may be.

6

Clobbering the events hash on inheritance is a limitation I have encountered before. So in favor of removing the events hash from Route.

One similar inconsistency that comes to mind is the eventManager hash on View.

7

+1 for removing/deprecating the events hash as I also think it’s not intentional to work with events in Route only. And getting _super within the defined methods is a big plus!

8

Thoughts on requiring event handler methods to be annotated as such? For example:

App.MyRoute = Ember.Route.extend({
  eligibleEventHandler: function(){
    // ...
  }.action(),
  notAnEligibleEventHandler: function(){
    // ...
  }
});
3 Likes
9

As I’m coming from a really great ‘convention over configuration’ server side framework (Grails) I love the idea of having a naming convention for event handlers as I think it makes your code more readable/understandable. So, I’ll give it a +1 :wink:

10

+1 to removing the events hash but having a convention to define an event.

Having a convention would also solve the issue of accidentally calling private APIs by doing something like {{action destroy}}

11

I’m not a huge fan. I’m thinking there’s probably some other framework mechanism for preventing inherited private API from being called, and it just seems kind of disruptive to the workflow to have to remember to tack .action or .event (as suggested in a PR) to the end of something that responds to send.

12

Annotating event handlers could allow for helpful error messages if a non-handler receives an event. However, it doesn’t solve the namespace collision issue: an annotated destroy handler will still override a destroy in the superclass, which will still lead to confusing behavior to debug. Overall, I’m not sure that the benefits outweigh the cost of being so declarative.

13

I was all in favour of deprecating until I realised things like e.g. destroy naming issues was mentioned. The real problem I’ve had is the ability to reopen classes and add events rather than clobber the existing hash. Something like this PR or a built in implementation of something like the following workaround would be good:

Em.Route.reopenClass
  reopenEvents: (events) ->
    @prototype.events ||= {}
    Em.merge @prototype.events, events
14

After some more thought, a simpler way to avoid (most) name collisions whilst not overcomplicating things with the event hash is adopting a naming convention like {{action destroy}} triggers event onDestroy. You’re less likely to have conflicts and it’s quite a well known convention in js anyway. This would be at odds with what happens in Ember.View events, but perhaps there’s an argument for those events to be changed as well? :wink:

1 Like
15

+1 for removing/deprecating the events hash.

I think that Routes, Controllers and Views should behave in the same way. And removing events makes the most sense to me primarily because:

  • Preserving the ability to call controller methods from JavaScript (@machty’s #3). I have a lot of methods that can be called both through JavaScript and through {{action}}s.
  • The code looks cleaner without the events property, and I believe that it will be less difficult for newbies to figure out where stuff should go.

I don’t see {{action destroy}} as a big issue. Especially since this has always been the case, and it actually could be intended behavior. I’ve seen people use other inherited methods such as {{action toggleProperty 'isEditing'}}. We could go Angular style and name all Ember properties starting with a $, like controller.$destroy() - juuuust kidding :trollface:

16

@stefan.penner has made the following proposal, which aims for consistency in how we treat and work with actions in Ember (click where it says “gist.github.com” to view the whole thing:

https://gist.github.com/stefanpenner/17b7e9a46fe14334c92a

17

Inspite of having the actions hash in controllers, components and views, what is the difference between having an actions hash in place of existing events hash ???

18

I don’t love this suggested change, but I could be convinced. I would prefer that we have a period of time where we let everyone define actions directly on the route object, and we blacklist core API and throw an error when an action is fired with a reserved word. If the community responds and tells us how horrible it is and how they can’t name their actions what they want to, then I’d be more amenable to ActionableMixin and requiring that all action handlers explicitly be marked as such, but my hunch is that the fears of a crowded namespace are overblown, and that we’re over-engineering a solution to this problem that ends up being just one more thing for a brain to remember, both for newbs and veterans.

2 Likes
19

@Selva events is the same as actions; the suggestion is to rename this hash in order to better align with the {{action}} handler that fires the event from templates.

20

I share @machty’s hunch about over-engineering. Views and controllers have been working fine without annotating event handlers. I can’t recall a namespace collision issue in all the time I’ve been working with Ember.

1 Like