Skip to content

Commit

Permalink
[BUGFIX beta] Avoid run.next in app.visit resolve handler.
Browse files Browse the repository at this point in the history
The prior comment has been pointed to by a few different people in the
FastBoot team meetings so I decided to dig into exactly what the TODO
was talking about. Specifically, why do we need a `run.next` before
resolving the visit promise?

The answer is basically that the `router.handleURL` transition promise
being resolved does not actually guarantee that rendering is completed.
Since the renderer schedules the actual revalidation and rendering into
the `render` queue, we must wait for that to complete before resolving
the promise.

This commit explains that in the inline comments, and changes the
implementation to use `run.schedule('afterRender', ....)` instead of
`run.next` (since resolving "after rendering is complete" is what we
actually care about).
  • Loading branch information
rwjblue committed Nov 8, 2016
1 parent e659bd6 commit 32a7966
Showing 1 changed file with 10 additions and 8 deletions.
18 changes: 10 additions & 8 deletions packages/ember-application/lib/system/application-instance.js
Original file line number Diff line number Diff line change
Expand Up @@ -241,20 +241,22 @@ const ApplicationInstance = EngineInstance.extend({

let router = get(this, 'router');

let handleResolve = () => {
// Resolve only after rendering is complete
let handleTransitionResolve = () => {
return new RSVP.Promise((resolve) => {
// TODO: why is this necessary? Shouldn't 'actions' queue be enough?
// Also, aren't proimses supposed to be async anyway?
run.next(null, resolve, this);
// Resolve once rendering is completed. `router.handleURL` returns the transition (as a thennable)
// which resolves once the transition is completed, but the transition completion only queues up
// a scheduled revalidation (into the `render` queue) in the Renderer.
//
// This uses `run.schedule('afterRender', ....)` to resolve after that rendering has completed.
run.schedule('afterRender', null, resolve, this);
});
};

let handleReject = (error) => {
let handleTransitionReject = (error) => {
if (error.error) {
throw error.error;
} else if (error.name === 'TransitionAborted' && router.router.activeTransition) {
return router.router.activeTransition.then(handleResolve, handleReject);
return router.router.activeTransition.then(handleTransitionResolve, handleTransitionReject);
} else if (error.name === 'TransitionAborted') {
throw new Error(error.message);
} else {
Expand All @@ -268,7 +270,7 @@ const ApplicationInstance = EngineInstance.extend({
location.setURL(url);

// getURL returns the set url with the rootURL stripped off
return router.handleURL(location.getURL()).then(handleResolve, handleReject);
return router.handleURL(location.getURL()).then(handleTransitionResolve, handleTransitionReject);
}
});

Expand Down

0 comments on commit 32a7966

Please sign in to comment.