Ember.js: beforeModel transitions out of order if a mixin is used that calls super without returning a promise

[nathanpalmer]

We noticed this on one of our production projects today and was a little difficult to track down. Essentially we noticed some ajax calls firing out of order. What seems to happen is when we're mixing in multiple things and they all override beforeModel that the very last mixin cannot simply call this._super() but it must check to see if it's a promise and resolve it.

Here is a jsbin to replicate it

App.DelayMixin = Ember.Mixin.create({
  beforeModel: function() {
    this._super();
    Ember.Logger.log("start beforeModel");
    return new Ember.RSVP.Promise(function(resolve, reject) {
      setTimeout(function() {
        Ember.Logger.log("resolve beforeModel");
        resolve();
      }, 2000);
    });
  }
});

App.PromiseMixin = Ember.Mixin.create({
  beforeModel: function() {
    Ember.Logger.log("promise");
    this._super();
    var promise = new Ember.RSVP.Promise(function(resolve, reject) {
        resolve();
    });

    return promise;
  }
});

App.IndexRoute = Ember.Route.extend(App.DelayMixin, App.PromiseMixin, {
  model: function() {
    Ember.Logger.log("start model");
    return ['red', 'yellow', 'blue'];
  }
});

If I swap out the App.PromiseMixin with this it works properly. However, this wasn't what I was expecting to have to do. jsbin

App.SuperPromiseMixin = Ember.Mixin.create({
  beforeModel: function() {
    Ember.Logger.log("array");
    var superPromise = this._super();
    var promise = new Ember.RSVP.Promise(function(resolve, reject) {
        resolve();
    });

    if (superPromise.then) {
      return superPromise.then(promise);
    } else {
      return promise;                         
    }
  }
});

This brings up another thought though. What do you think about turning beforeModel and afterModel into events so we don't have to make sure that _super() is called in the right way. The way it is currently you could have a 3rd party library (which is what our case was) change the order of operations on your regular application code.

[mixonic]

I expect most apps want beforeModel hooks to run in a deterministic order- making them events would dodge the problem you have now (needing to care about what your parent class does) and replace it with another (how to change what or when your parent class does something).

For your sub-class to decide when its own logic will run (before or after the parent) the _super pattern feels correct. IMO if your addon was stomping on its parent, the addon has a bug.

Going to close this, since it does not read like an issue to me, but please let me know if you have any questions or feedback. This stuff is a little tricky!

[nathanpalmer]

@mixonic We don't need the beforeModel's to run in any particular order. However we do need the model events to run in a specific order.

beforeModel -> model -> afterModel

Without being able to rely on that we can't guarantee that something put in beforeModel actually runs _before the model_. Does that make sense? Take a look at the attached jsbin's in my description. You'll see that it's hitting model before the beforeModel has finished. It's not waiting on the promise.

[rwjblue]

beforeModel returns a promise (which is the result of this._super()), if you disregard the promise that is returned, there is no way to guarantee things wait appropriately. At a framework level when mixins are merged and a factory is created, there is no way to know which functions are async so we cannot enforce that they are run in series automatically.

tldr; If you override a method that is async and call this._super you must also chain off of its return value to avoid breaking the "promise" that beforeModel finishes before model is called.

Updated JSBin: http://jsbin.com/sunise/1/edit?js,console,output

[rwjblue]

Also, just to clarify, this has nothing to do with the last mixin: the rule applies any time that you call this._super in an async hook (from a mixin or not).

[nathanpalmer]

@rwjblue yup, that makes sense and how we got around the problem. It would be great if beforeModel and afterModel were simply events so the semantics around this were dealt with within the framework itself and not a potential issue hidden behind addons abstraction.

For example here is the beforeModel used by simple-auth which I'm sure is fairly widely used within the community

• https://github.com/simplabs/ember-simple-auth/blob/7951915cc5366536f4273a83fd910954441b00d9/packages/ember-simple-auth/lib/simple-auth/mixins/authenticated-route-mixin.js

It doesn't attempt to deal with async nor did I see it in the documentation which is why this caught me by surprise (eventhough it makes perfect sense why it would behave that way.)

[rwjblue]

Submitted https://github.com/simplabs/ember-simple-auth/pull/464 to fix ember-simple-auth.

The async routing docs don't discuss calling this._super at all. I think this sort of detail likely belongs in the API docs (not the guides), but that is more of a @trek question. Maybe open an issue at https://github.com/emberjs/guides to see if we should flesh out that guide with details on inheritance...

[nathanpalmer]

Thanks @rwjblue! I appreciate you jumping in and fixing the simple-auth issue. That will provide a good example for others too.

Looking forward what does the team think about getting away from these as overrides and move them to events. We already have things like willTransition that you can use within actions. Even init you can use the _on_ syntax see don't override init. I'd love to abstract away these issues. Something like this

setupBeforeModel: function() {
  // async stuff
}.on('beforeModel')

or even this

actions: {
  beforeModel: function() {
  }
}

would give authors enough of a callback to use while letting ember handle an RSVP.all around all of them.

[rwjblue]

We are talking about removing beforeModel and afterModel (details in the routable components rfc). Making them events or actions doesn't seem to solve the same problem, ordering of these hooks really does matter in most cases.

I suppose I would be ok with it also being an event, but it doesn't seem like a clear benefit to me...

[aaronbhansen]

The before and after model hooks are great for performing route centric logic, but it also means that every mixin added to the route today, that uses the hooks, has to properly implement the hooks calling super and handling the parents returned promises. If the hooks are events, they become agnostic mixins that can perform their logic within their own event, return a promise or not, and then ember can coalesce all the returned values and appropriately handle resolving all returned promising before continuing onto model. Just as the pattern with not overriding init is a good practice, I think any thing ember can do to remove overriding methods and encourage developers to use events or actions, will ensure later that as the ember ecosystem grows, conflicts are removed and hooks are isolated to the mixins or objects themselves. Developers will no longer need to call super and then handle the results appropriately, because embers framework will do that.

[rwjblue]

@aaronbhansen - As I mentioned above I'm not opposed, I just don't see it as a clear benefit. I think folks would have the same amount of issues regarding ordering (since these hooks are often used for authentication incorrect ordering could be a security concern) as they do today about not calling this._super.

Just to re-iterate: I'm happy to review a pull-request that adds this functionality. We can then discuss the merits of the approach and any cons associated. I'm pretty sure we will never convince each other of anything with imaginary code....