Skip to content

Commit

Permalink
Fix followRedirects when source is async and destination is sync
Browse files Browse the repository at this point in the history
The previous implementation of `followRedirects()` would catch a transition rejection and check the router for an `activeTransition`. This can become problematic in async situations because the destination transition may have resolved before the `reject` is scheduled on the previous transition. One such case is when redirecting from an async model hook to a destination route with synchronous model hooks.

This commit updates the `followRedirects()` logic to explicitly follow the redirect chain rather than relying on the presence of an `activeTransition`. This makes following redirects work correctly regardless of any scheduling concerns.

This problem has been noted in the context of the `visit()` test helper:

- emberjs/ember-test-helpers#332
- emberjs/ember.js#17150
  • Loading branch information
davidtaylorhq committed Nov 16, 2023
1 parent 9320c7b commit 787cfbe
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 4 deletions.
10 changes: 6 additions & 4 deletions lib/router/transition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export type OpaqueTransition = PublicTransition;
export const STATE_SYMBOL = `__STATE__-2619860001345920-3322w3`;
export const PARAMS_SYMBOL = `__PARAMS__-261986232992830203-23323`;
export const QUERY_PARAMS_SYMBOL = `__QPS__-2619863929824844-32323`;
export const REDIRECT_DESTINATION_SYMBOL = `__RDS__-2619863929824844-32323`;

/**
A Transition is a thenable (a promise-like object) that represents
Expand Down Expand Up @@ -71,6 +72,7 @@ export default class Transition<R extends Route> implements Partial<Promise<R>>
isCausedByAbortingReplaceTransition = false;
_visibleQueryParams: Dict<unknown> = {};
isIntermediate = false;
[REDIRECT_DESTINATION_SYMBOL]?: Transition<R>;

/**
In non-production builds, this function will return the stack that this Transition was
Expand Down Expand Up @@ -309,6 +311,7 @@ export default class Transition<R extends Route> implements Partial<Promise<R>>
}

redirect(newTransition: Transition<R>) {
this[REDIRECT_DESTINATION_SYMBOL] = newTransition;
this.rollback();
this.router.routeWillChange(newTransition);
}
Expand Down Expand Up @@ -419,10 +422,9 @@ export default class Transition<R extends Route> implements Partial<Promise<R>>
@public
*/
followRedirects(): Promise<R> {
let router = this.router;
return this.promise!.catch(function (reason) {
if (router.activeTransition) {
return router.activeTransition.followRedirects();
return this.promise!.catch((reason) => {
if (this[REDIRECT_DESTINATION_SYMBOL]) {
return this[REDIRECT_DESTINATION_SYMBOL]!.followRedirects();
}
return Promise.reject(reason);
});
Expand Down
29 changes: 29 additions & 0 deletions tests/router_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4682,6 +4682,35 @@ scenarios.forEach(function (scenario) {
});
});

test('Transition#followRedirects() works correctly when redirecting from an async model hook', function (assert) {
assert.expect(2);

routes.index = createHandler('index', {
beforeModel: function () {
return Promise.resolve(true).then(() => {
return router.transitionTo('about');
});
},
});

routes.about = createHandler('about', {
setup: function () {
assert.ok(true, 'about#setup was called');
},
});

router
.transitionTo('/index')
.followRedirects()
.then(function (handler: Route) {
assert.equal(
handler,
routes.about,
'followRedirects works with redirect from async hook transitions'
);
});
});

test("Returning a redirecting Transition from a model hook doesn't cause things to explode", function (assert) {
assert.expect(2);

Expand Down

0 comments on commit 787cfbe

Please sign in to comment.