New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

All transitions are now async #19

Merged
merged 1 commit into from Jun 16, 2013

Conversation

Projects
None yet
@machty
Collaborator

machty commented May 19, 2013

Please refer to https://gist.github.com/machty/5723945 for a description of the API

handler.deserialize(handlerInfo.params, providedContext, resolvedContexts)
).then(function(context) {
contextResolvedImmediately = true;

This comment has been minimized.

@stefanpenner

stefanpenner May 19, 2013

Collaborator

this should likely be named contextHasFulfilled

// unnecessarily enter the loading state.
RSVP.resolve().then(function() {
if (!contextResolvedImmediately) {
loading(router);

This comment has been minimized.

@stefanpenner

stefanpenner May 19, 2013

Collaborator

shouldn't this also guard against an earlier rejection?

if (!(e instanceof Router.TransitionRedirected)) {
failure(router, e);
throw e;
}

This comment has been minimized.

@stefanpenner

stefanpenner May 19, 2013

Collaborator

the else clause does not rethrow or return a rejected promise, so the downstream thenable becomes a success, fulfilled with undefined. I am unsure if this is intended.

@darthdeus

This comment has been minimized.

darthdeus commented May 19, 2013

❤️

@teddyzeenny

This comment has been minimized.

Contributor

teddyzeenny commented May 23, 2013

Thanks @machty for this awesome improvement!

@machty

This comment has been minimized.

Collaborator

machty commented May 23, 2013

Hi, I've made some major updates to this PR, mostly to define the beforeModel/model/afterModel hooks here (rather than the original plan of preserving while bloating deserialize and letting Ember split out the hooks).

It's still not pull-ready. Here's a TODO:

  • Given that all transitions are async, how can we (should we?) preserve global Loading state behavior? Ideally we should prevent the loading state from entering if the transition promises resolved "immediately", but this is router.js, and we don't have a run loop to help us determine what 'immediately' is.
  • What about Failure state? With this PR you have much better control over how to handle errors... perhaps we don't want a global Fail state any longer?
  • Given the complexity, should we put in a system for logging in this PR or save it for later/Ember?
  • Re-write README
  • Better docs
@ryanflorence

This comment has been minimized.

ryanflorence commented May 23, 2013

I think this is a step toward loading partial applications <3

@stefanpenner

This comment has been minimized.

Collaborator

stefanpenner commented May 23, 2013

@rpflorence c señor.

@ahawkins

This comment has been minimized.

ahawkins commented May 24, 2013

@machty you are doing god's work

@machty

This comment has been minimized.

Collaborator

machty commented May 25, 2013

Here's a nice state of the union API usage gist for how to handler some (presently) challenging async use cases: https://gist.github.com/machty/5647589

Please, everyone who cares about this, have a look and let me know if there's anything about the above gist that looks awkward that could be fixed/improved about this API. We don't need to fit every last thing in Ember Core; this gist is just to outline some ideas about possible approaches.

@drogus

This comment has been minimized.

drogus commented May 25, 2013

I will look at those changes closer more thoroughly tomorrow, but one thing that I wanted to note is that LoadingRoute never really worked for me without hacks in Ember, so I guess we don't have to worry about breaking anything in that area. If I'm wong and it should work, please let me know what I'm doing wrong: http://jsfiddle.net/drogus/xZXux/7/, it's simple notes app (which does not really work anyway, beacuse I implemented mostly routing, but it works to the point when it needs loading route). When LoadingRoute is defined, there is a "mazximum call stack exceeded error": http://fiddle.jshell.net/drogus/xZXux/7/show/#/notes/1 I didn't have time to look into it closer.

@marcioj

This comment has been minimized.

marcioj commented May 25, 2013

@drogus you have to change the render hook to renderTemplate:

App.LoadingRoute = Ember.Route.extend({
    renderTemplate: function() {
        this.render('loading');
    }
});

But this will generate other problem, give a look in that jsfiddle.
Because router#targetHandlerInfos isn't initialized, and it is just initialized when setupContexts is called.
I think initializing this.targetHandlerInfos = []; here would solve the problem.

@machty

This comment has been minimized.

Collaborator

machty commented May 26, 2013

Hi, progress: http://jsbin.com/axarop/2/edit

This is the latest router.js as included in Ember. Haven't pushed this code you but you can start trying out the new API.

@drogus

This comment has been minimized.

drogus commented May 26, 2013

@marcioj ah, yes, of course it should be renderTemplate, thanks for correction. When I was quickly changing this code to make it throw an error I haven't noticed that this is not the error I'm looking for. Indeed, the error you mentioned was the one that I saw earlier.

@machty machty referenced this pull request May 26, 2013

Merged

Async transitions #2740

@jamesarosen

This comment has been minimized.

jamesarosen commented May 29, 2013

Does enter get the promise or what the promise resolves with?

@machty

This comment has been minimized.

Collaborator

machty commented May 29, 2013

@jamesarosen Nothing's changing about the API in that sense, so, same as before, enter (actually activate, which is the public api for Ember) won't get called with the context, but setup (private api) will get called with the resolved context, which will be used to set the model value on the controller.

@jamesarosen

This comment has been minimized.

jamesarosen commented May 29, 2013

I must have gotten a little confused reading the source.

https://github.com/tildeio/router.js/blob/master/lib/router.js#L216-L229 suggests that if deserialize returns a "thennable", it sets the context to the "thennable" object. https://github.com/tildeio/router.js/blob/master/lib/router.js#L497 and https://github.com/tildeio/router.js/blob/master/lib/router.js#L507 show that setup is called with context directly.

On the other hand, https://github.com/tildeio/router.js/blob/master/lib/router.js#L418 shows that proceed is called with the result of the promise.

@machty

This comment has been minimized.

Collaborator

machty commented May 29, 2013

@jamesarosen welcome to the world of inconsistency that is present router.js. This PR makes it so that both transitionTo and URL transitions will pause for promises and pass the resolved context to setup.

@machty

This comment has been minimized.

Collaborator

machty commented May 31, 2013

Hi, made another update. (Excuse the broken build for now, just a minor thing from a rebase). Check out this gist for the latest.

@joelmoss

This comment has been minimized.

joelmoss commented Jun 2, 2013

Is this PR likely to be merged before Ember 1.0 final is released?

@machty

This comment has been minimized.

Collaborator

machty commented Jun 2, 2013

Yes, probably rc6

Sent from Mailbox for iPhone

On Sat, Jun 1, 2013 at 8:35 PM, Joel Moss notifications@github.com
wrote:

Is this PR likely to be merged before Ember 1.0 final is released?

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

@stefanpenner

This comment has been minimized.

Collaborator

stefanpenner commented Jun 2, 2013

confirmed, it will, at least in one form or another.

@machty

This comment has been minimized.

Collaborator

machty commented Jun 14, 2013

OK! I am happy with it. Ready to merge when yall are. Speak now or forever hold your peace.

@joelmoss

This comment has been minimized.

joelmoss commented Jun 14, 2013

👍

@KidkArolis

This comment has been minimized.

Contributor

KidkArolis commented Jun 14, 2013

If I were to use tildeio/router.js in a non ember project, should I be using the version from this PR? As in, this is gonna be the next version, right?
It would be really nice if the examples in the README were updated to demonstrate some of the new stuff (I think the gists you've linked to only contain Ember specific examples) (I guess I can refer to the tests in the meantime).

@machty

This comment has been minimized.

Collaborator

machty commented Jun 14, 2013

@KidkArolis you're totally right, I completely forgot to rewrite the README, will do so soon. But yes this will be in the next version.

@KidkArolis

This comment has been minimized.

Contributor

KidkArolis commented Jun 14, 2013

One thing I've noticed so far is that, because it's heavily promisified now, it's hard to know when a 'programmer error' happend somewhere in the handler. I can see the errors if I turn the logging on, but sometimes you don't want to see detailed logging, however you always (in dev) want to see things like TypeError: Cannot call method 'html' of undefined which happened in my handler.model. Stack trace is also very useful in that case. Normally you could just log all errors and the stack, but I think I saw you've mentioned somewhere that transitions can be aborted by throwing an exception in the beforeModel hook, etc. I guess in that case you want to distinguish 'programmer errors' and exceptions thrown on purpose. I know when.js has a debug mode which is very helpful in dev with it's loud error logging, perhaps rsvp has something similar?

To sum up, I might be wrong, but I think this promise heavy code might lead to a lot of silent errors (thinks like undefined functions, type errors, etc.) that Ember developers will stop seeing, which might lead to a lot of frustration, so might be worth investigating.

Perhaps it could be as simple as having a flag to console.error(e.stack) in the handleError function.

@machty

This comment has been minimized.

Collaborator

machty commented Jun 14, 2013

@KidkArolis this is a problem we are aware and we're working at some better patterns for making things as transparent as possible. In the Ember world, we have a default error handler installed on the root route that spits out an error, but even then the stack is already lost at that point. So, anyway, there's lots of improvements to be made along the lines you're suggesting, but for now the need for some cohesive, consistent, and reliable patterns when it comes to routing outweighs the present difficulties with debugging, which we're pretty certain will become less and less problematic over time.

@KidkArolis

This comment has been minimized.

Contributor

KidkArolis commented Jun 14, 2013

One more question, sorry for the noise. At the moment, it's not possible to abort/pause a transition from within an active state which is about to be exited, is that correct? It could be quite easily added by passing the transition into a beforeExit like method that's called on each handler that's about to be exited in reverse order before even proceeding with deserialization (only asking if I'm getting it right).

The usecase is displaying a modal asking the user if he really wants to navigate away from the page, etc.

@KidkArolis

This comment has been minimized.

Contributor

KidkArolis commented Jun 15, 2013

Ah! Read your gist and saw the willTransition hook!

Question though, do you think it should call willTransition on a reversed list of the handlerInfos? I feel that a state that was entered last should be the first to get a chance to prevent the transition by displaying something in the UI, etc.

Also if a handler prevents the transition in willTransition, it seems like all the other handlers will still get called with willTransition even though it was already aborted? I guess that's ok, cause the willTransition in ather handlers can check for that and decide not to do anything.

@KidkArolis

This comment has been minimized.

Contributor

KidkArolis commented Jun 15, 2013

May I ask, what's the reason for splitting deserialize into beforeModel/model/afterModel? It seems like it would be pretty easy for ember to split out the hooks into whatever it needs. Those 3 methods are not really doing anything different functionally (in some sense). At least in my case, I'm not using the 'model' hook for loading model per say as it's used in Ember, I rather create a State object in there (which is kind of a model), but deserialize sounded .. more abstract, and this model concept leaks a bit of ember into the router. Just something that crossed my mind.

@machty

This comment has been minimized.

Collaborator

machty commented Jun 15, 2013

@KidkArolis Yes, use the willTransition event to give active routes an opportunity to prevent/redirect/decorate a transition. Like all other events, it'll be fired on the leaf route and bubble upward to root (but unlike default events it won't throw an error if no one handles it) so you'll get the reverse behavior you're asking about.

Events won't bubble unless you explicitly return true from an event handler. It doesn't have anything to do with whether the transition event was aborted (but you probably won't want to return true if you aborted a transition in willTransition).

It's split into 3 because model won't be called if you call transitionTo('foo', fooModel), and it seemed to make more sense to preserve these naming conventions so that router.js is useful/comprehendible as a microlib rather than just expose a single method called getEntryPromise (which was one of the original stabs at this API).

@KidkArolis

This comment has been minimized.

Contributor

KidkArolis commented Jun 15, 2013

Thanks for the clarifications! This library is amazing, especially the new version with transitions. Using it in a Backbone atm - I'm finding this state machine approach to be a very good way to structure the app, very glad you guys have extracted the router into a microlib.

@stefanpenner

This comment has been minimized.

Collaborator

stefanpenner commented Jun 15, 2013

@KidkArolis pretty cool. That sounds like a great blog post in the making, please keep us in the loop.

All transitions are async
API:
https://gist.github.com/machty/5723945

Examples (might have some examples from previous iterations):
https://gist.github.com/machty/5647589

stefanpenner added a commit that referenced this pull request Jun 16, 2013

Merge pull request #19 from machty/async-transitions
All transitions are now async

@stefanpenner stefanpenner merged commit 5d4d4bb into tildeio:master Jun 16, 2013

1 check passed

default The Travis CI build passed
Details
@stefanpenner

This comment has been minimized.

Collaborator

stefanpenner commented Jun 16, 2013

o_0)

lcoq pushed a commit to lcoq/ember.js that referenced this pull request Nov 8, 2013

Async transitions
This is the Ember side of
tildeio/router.js#19

All set, just keep in mind that there is probably more discussion to be
had over the `beforeModel` and `afterModel` hook names, which might
change before RC6.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment