Async transitions

[machty]

Async-ify the router.

API guide
possibly out of date but live examples
router.js PR

Likely to make it into RC6

[machty]

Let's merge this thing.

[KasperTidemann]

Oh, yes! Great work.

[ahawkins]

@machty is doing god's work. bows.

[tomdale]

Does this break existing apps that rely on the redirect hook?

[josepjaume]

Oh yes, this is a game changer for my app 👍

[darthdeus]

Oh yeah baby, please merge this goodiness

[machty]

@tomdale Take a look at https://github.com/emberjs/ember.js/pull/2740/files#L5R260

I think this will Just Continue to Work for most people's apps; redirect is a hook that gets passed the resolved model and allows you to abort the transition by calling transitionTo, and it's those semantics that I've preserved, but I won't be surprised if some apps with subtle dependencies on the sync-ness of transitions get bitten, but I'm confident I can knock out (or at least obviate and better document) a majority of the surprises that come up for the folk living off master before we release RC6. Thoughts?

edit: One other criterion of redirect is that it is called within setup, after previous routes have already been added to currentHandlerInfos, which will affect things like sending events within redirect, but I'm wary of 1) preserving the pre-facelift conceptual blurriness of allowing the router to internally be sort of in between two states (much if not all of the resulting whack-a-mole has been well encapsulated in Transition) and 2) having afterModel and redirect exist but have subtle assumptions under which they're called. But I guess point (1) would also apply to per-route loading states, and everyone wants those, so maybe that point's invalid. BUT, fwiw, I can totally preserve redirect being called from within setup if that truly is the best option, but I'm leaning toward keeping things as they are and addressing the feedback from those living on master, and we can move redirect to setup at any point.

[workmanw]

This looks potentially breaking, but sooooo worth it.

Merge Please

[machty]

Worth discussing: I removed the global FailureRoute and assimilated its test cases into testing the error hook, which allows for finer grain control of errors, and given the fact that FailureRoute isn't mentioned anywhere on Emberjs.com, or anywhere on StackOverflow, I don't think this will be a huge shock to people.

If you really want to define default error handling that will fire for ALL routes, you can do as follows

Ember.Route.reopen({
  error: function(reason, transition) {
    // optionally call super to preserve default logging.
    // this.super(reason, transition);
    this.transitionTo('helpme');
  } 
});

This seems more intuitive (albeit new) to me than making using of FailureRoute... which isn't really a route and people only used it for redirecting anyway... right?

[stefanpenner]

@machty should the ApplicationRoute's implicitly error handler, simply transition to the default FailureRoute ?

[machty]

@stefanpenner the error hook doesn't bubble up the route hierarchy; it's handled directly on the route where the error occurred (e.g. a promise rejected in beforeModel/model/afterModel/setup on a particular route).

[workmanw]

@machty I personally agree your change. In our app, having a single FailureRoute is a bit of a smell, what ends up happening is we have this single route that is checking 4 or 5 different models in order to figure out what error message to show the user.

The only thing I would like to see is the error bubble up.

[stefanpenner]

@workmanw there is nothing wrong with a catchall.

@machty any reason to not bubble this?

[machty]

@workmanw @stefanpenner it means that error handlers would have to defined on the events hash, which hurts the simplicity/grokkability of the API. You have beforeModel/model/afterModel all defined on the Route itself, but once you want to start handling errors, you have to put the error on the events hash. Not sure how much of a dealbreaker that'd be.

We could also not put it on the events hash and have it still bubble, but then that's inconsistent with the only other thing on Routes that bubble: events.

[workmanw]

@stefanpenner Agreed, there is certainly nothing wrong with a catch all, but what I was trying to say is having a catch all as you're only option for catching stuff is smelly. Which is how it currently works (AFAIK). If we could bubble errors, your catch all could live at the ApplicationRoute and this would all feel pretty consistent to me.

@machty What if instead of bubbling with events and trigger, the default implementation of error invoked error on the parent route's handler (assuming you can easily obtain the parent route handler). This would create a natural ancestor bubbling effect. Ex:

Ember.Route = Ember.Object.extend({
  // ...
  error: function(error, transition) {
    if(this.parentRoute) this.parentRoute.error(error, transition);
  }
});

[machty]

@workmanw it's presently non trivial to get the parent route (involves a loop on router.js's internal structure). I also don't want to weirdly get stuck between inheritance-based polymorphism and event-based polymorphism. Seems like that'll just be confusing.

[workmanw]

@machty, Yeap. That's fair.

[machty]

Seems like we're going to put error on the events hash.

[workmanw]

@machty Thanks for all you're hard work.

[machty]

Your welcome!

[sebastianseilund]

Awesome! This will make a lot of things way more elegant in our app. Great work Alex!

[machty]

Just pushed a new version with tons of documentation and bubbling error events.

[ghedamat]

Awesome!

[stefanpenner]

@machty 💃

[lukemelia]

Can we/should we expose a logging hook, for debugging what the router is doing? It will make any issues people run into hella easier to debug.

[lukemelia]

I tried updating an app with a complex router and found a bug with how params are being handled when you have multiple dynamic segments. Demonstration here: http://jsfiddle.net/DCrHG/77/

It may be related to this code:

https://github.com/machty/ember.js/blob/router-promises/packages/ember-routing/lib/system/route.js#L410-416

Or perhaps the params were previously being filtered before passed to model?

[bradleypriest]

I can't even begin to explain how excited I am about this, being able to halt transitions if an object is dirty is going to remove so much ugly code from my codebase 🍺

[machty]

@lukemelia I'll look into that bug shortly, surprised router.js test suite wouldn't have caught it.

router.js has the log hook which has been pretty helpful for me to see what's going on. How should we expose it on the ember side? We already have LOG_TRANSITIONS, would we add LOG_TRANSITIONS_VERBOSE or something?

[machty]

I'm free all day. Where/when yall wanna meet?

[dgeb]

This PR looks solid and is so very much appreciated, @machty. I think my only concern is semantic: the purpose of beforeModel and afterModel is not clear in the same way that setupController, renderTemplate, and model are.

I propose refactoring beforeModel -> verify and afterModel -> verifyModel.

[machty]

Previous iterations of this facelift had prepareTransition, model, and validateTransition, but many, including myself, were finding these names cumbersome and there was a lot of pushback to go back to the less semantic but brutally clear beforeModel/model/afterModel. I'm alright with it since model is the main semantic centerpiece which is remaining unchanged, with necessary helper hooks beforeModel and afterModel to support it that leave no ambiguity as to when they're called and why you would use one vs. the other. Adding verify, verifyModel, prepareTransition, validationTransition just adds more vocabulary for the brain to keep track of IMO.

[darthdeus]

agreed, there are already sooo many vocab things to remember :/ like render outlet into blah template
—
Sent from Mailbox for iPhone

On Sun, Jun 16, 2013 at 2:52 PM, Alex Matchneer notifications@github.com
wrote:

Previous iterations of this facelift had prepareTransition, model, and validateTransition, but many, including myself, were finding these names cumbersome and there was a lot of pushback to go back to the less semantic but brutally clear beforeModel/model/afterModel. I'm alright with it since model is the main semantic centerpiece which is remaining unchanged, with necessary helper hooks beforeModel and afterModel to support it that leave no ambiguity as to when they're called and why you would use one vs. the other. Adding verify, verifyModel, prepareTransition, validationTransition just adds more vocabulary for the brain to keep track of IMO.

Reply to this email directly or view it on GitHub:
#2740 (comment)

[dgeb]

Ember's API already has quite a surface area, yet it remains accessible because method names are fairly consistent with their intended usage. I think that naming methods based upon their order of operations, rather than their intentions, is a break from a formula that's worked well so far. I would argue that the challenge isn't to limit vocabulary, but to ensure that new methods remain semantic and consistent.

I know naming issues can be a bit subjective, so I try to think from the perspective of someone new to Ember. Would they find it more clear to learn that you verify the accessibility of a route in verify or beforeModel? Or that you verify that a route's model is accessible in verifyModel or afterModel?

[machty]

When you ask the question "which method would you use to verify the accessibility of the route, a) verify or b) beforeModel?" everyone would obviously choose (a), but that's obviously a very primed way of presenting the API. My thinking is that we don't present all the hooks as one giant blob of equally prioritized hooks, but rather present them in the order that most people approaching ember might quickly adopt, so, just a before, we'll present all the different hooks, keeping model as the hook that resolves the model for the route. We can mention beforeModel and afterModel just to put it in the back of people's minds, so that when they start building an app and they realize "hmm, I need to run some logic, possibly async, that needs to happen before trying to resolve the model... what method should I use?" Obviously another primed example, but I think more closely in line with how people will search for such a hook when their use case is not met by the others, and verify and verifyModel are both pretty bad answers to the question "Which hook can you use before the model of a route is resolved?", which is the question that's been asked repeatedly in various GitHub issues when people have been thwarted by the default (and inconsistent ordering) of model resolution vs the redirect hook.

[josepjaume]

What about beforeFilter and afterFilter? Would that be acceptable?

[machty]

No, people would confuse that with enter/exit, or some other lifecycle hook not specific to model resolution.

[lukemelia]

spelling: "defaut"

[drogus]

Apart from above discussion, I have one thing to ask for: after merging this, can we have some time before releasing another rc version? This is a big change and although it is awesome, I'm pretty sure that we will hit some problems soon, with next rc pushed quickly it will be hard to do any changes that need API changes.

[machty]

@drogus tis the plan

[ghempton]

Woot! This is a big win for ember. Nice work @machty!

[jonnii]

🚢 🚢 this is awesome, let's get an RC out!! =)

[kiwiupover]

Thanks for the hard work @machty. I'm sure an embercast would be helpful. Cheers

[stefanpenner]

@jonnii we wil give this merge a few days to settle before another RC. If all goes well, we will see another RC in a weeks time.

[rlivsey]

Just upgraded Ember in my (fairly substantial) app and everything still works, now to start deleting lots of code which is no longer needed!

Awesome stuff @machty 🍻

[machty]

Relief!

[bradleypriest]

Just upgraded my quite substantial app too.

Only a couple of gotchas completely unrelated to the code itself

• Having to manually resolve an Ember-Data ManyArray in the model hook if the parent had just been created (Definitely an ED issue, not a router issue)
• Remembering to differentiate and flag discarded data due to willTransition being called twice when aborting then retrying a transition when throwing an Are You Sure? dialog on a dirty form.

Bravo @machty. This is awesome.

[stefanpenner]

@bradleypriest we ran into the same issue with ember-data, I have some time tomorrow planned to look into it. Hopefully it will lead to a fix.

[machty]

@bradleypriest What do you think is a good solution / pattern for preventing the willTransition gotcha?

In a similar case, I opted for disabling the willTransition on transitions started while another was is already active to avoid this sort of problem. Are you using .retry()?

[teddyzeenny]

@bradleypriest @stefanpenner I've had an open PR in ED about the ManyArray issue for a while emberjs/data#957

[bradleypriest]

@machty yep, I'm calling .retry() on the transition.
I'm running some slightly convoluted teardown logic with managing child models before rerunning the transition and that is really the only reason I've needed to work around it.
I don't think I'm running into any real issue here, but who knows what other people will run into it with

[bradleypriest]

@machty Is there are a pattern for returning a promise object but not waiting for it to resolve?
i.e. Returning an ED many-array without blocking the UI?

I'm currently using

model: function(params, transition) {
  comments = transition.resolvedModels.post.get("comments")
  Ember.run(comments, 'resolve', comments)
  comments
}

But I'm not sure if there are other promises this is resolving early within ED

[drogus]

@bradleypriest promises on collections and models will be removed in ED in the future

[machty]

@bradleypriest confirm what @drogus said. It might just be best to reopen the various ember data Model classes and set their .then properties to null, or by doing some magic that moves their .then properties to another property called loadingPromise or something.