Delay Routing to Wait for Loading #1183

Merged
merged 3 commits into from Jul 21, 2012

Projects

None yet

4 participants

Owner
wagenet commented Jul 18, 2012

This should generally work now.

This pull request passes (merged 37a22263 into c27a310).

Owner
wagenet commented Jul 18, 2012

Fixes #1151

This pull request passes (merged 571224f5 into c27a310).

This pull request passes (merged 20f22944 into c27a310).

This pull request passes (merged ac3c2bb2 into 44041af).

This pull request passes (merged 845d90e5 into 44041af).

Member

This is a cool feature that could solve lots of problems. My big question would be why is the routeManager locked while the promises are fulfilled? This seems bad since it will essentially prevent the user from doing lots of things until the models are loaded. I think it is perfectly reasonable for the user to navigate away before the model is loaded.

Owner
wagenet commented Jul 19, 2012

The idea is that you shouldn't be able to send any events while in the loading state. However, forward and back should probably work still. We'd also need to make sure we cancelled the promise execution in that case. I'll have to think about this a bit more.

This pull request fails (merged d4a3fc04 into 44041af).

This pull request passes (merged c5410ec9 into 0a4ece1).

Member

I also think you should be able to send other events before the routing is finished. An example would be a tab navigation: seems weird to disable the navigation until the tab is loaded. In general, this would disable navigation site-wide (excluding forward and back) until the model data is loaded, no?

Owner
wagenet commented Jul 19, 2012

@ghempton That's correct. I don't think we can predictably handle any events until the route actually loads. We're in an indeterminate state until it loads. Feel free to explain to me where I'm wrong.

Member

Well that is a result of the implementation. The async part of the routing e.g. loading model data could be considered to have no side-effects. The actual transition wouldn't start until all the async stuff is finished. In this case, the state manager could still be considered in the old state and all events could work as expected (pending transitions cancelled if a new one happens).

I had this same issue in route manager and solved it with a similar method.

Owner
wagenet commented Jul 19, 2012

@ghempton What about the case where this route was navigated to directly? Also, we need to have a way to show users that something is happening. Just clicking on a link and seeing nothing happen is not good user experience.

Member

Is there a difference when the route was navigated to directly between the implementations? In either case, the user needs to wait for the models.

There could be a flag on the state manager isPending to indicate a pending transition. This could be used to indicate something is happening to the user. I think having the UI block for potentially hundreds of milliseconds is not a good user experience either.

Owner
wagenet commented Jul 19, 2012

@ghempton If we don't block events, we need to have a good way to cancel any pending loads. One possibility would be to have any transitionTo abort the context resolution. It's possible that doing this in conjunction with an isPending flag would work alright. I'll play around with it.

This pull request passes (merged e888ce97 into 620e78b).

This pull request passes (merged c140bc2b into 620e78b).

This pull request passes (merged 83debec5 into 620e78b).

This pull request passes (merged 1b3e95f1 into 620e78b).

This pull request passes (merged fa7c4abe into 620e78b).

This pull request passes (merged 45a306d8 into 76af834).

Owner
wagenet commented Jul 20, 2012

@ghempton I talked with @wycats about this some and he reminded me that this code only comes into play when we route via the url. "href actions" don't route via the url. This really only matters when entering the app at a specific path/hash. In this case, I think it does make sense to transition to a loading state and lock any events. However, we still should allow forward/back navigation in the rare case that the user directly navigate to two different urls in the same app.

This pull request passes (merged 38201dc9 into 76af834).

Member

Ok. What was the issue this was solving for again?

Owner
wagenet commented Jul 20, 2012

@ghempton The issue is when you navigate directly to a URL that has multiple levels of context where the latter are dependent on the former. I do not think it's a very large case, but it does exist and is very surprising when it doesn't work properly.

Member

That case definitely makes sense.

Can't help but think that there is a more general need to async load resources during state transitions (e.g. perform redirection logic against a fully loaded model inside of connectOutlets), but I know you guys took the async transition logic out of state manager for a reason.

Member

Here is an example of where we would need async routing: https://gist.github.com/3152166

This pull request passes (merged eb37c382 into 2d9f107).

This pull request passes (merged 852f41f into ef3ceae).

This pull request fails (merged d39546c3 into ef3ceae).

@wagenet wagenet merged commit a567b35 into emberjs:master Jul 21, 2012

This pull request passes (merged f46cd83 into ef3ceae).

@wagenet or @ghempton is there an example of this??, o maybe a fiddle??

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment