Skip to content
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.

Sign up for GitHub

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

Jump to bottom

v3.6.0-beta.1: redirect causes visit test helper to fail #17150

Open
buschtoens opened this issue Oct 24, 2018 · 6 comments
Open

v3.6.0-beta.1: redirect causes visit test helper to fail #17150

buschtoens opened this issue Oct 24, 2018 · 6 comments
Labels
Bug Needs Reproduction Regression Router Bugs

Comments

@buschtoens
Copy link
Contributor

buschtoens commented Oct 24, 2018 •
edited
Loading

I have a test case like:

test('visiting `/retirement`', async assert => {
  await visit('/retirement');

  assert.strictEqual(
    currentURL(),
    '/retirement-check',
    'redirects to `/retirement-check`'
  );
});

Prior to v3.6.0-beta.1 (v3.5.0 to be precise) this test worked.
Now it fails with a TransitionAborted error. Adding a noop catch clause fixes it:

test('visiting `/retirement`', async assert => {
  await visit('/retirement').catch(() => {});

  assert.strictEqual(
    currentURL(),
    '/retirement-check',
    'redirects to `/retirement-check`'
  );
});

Is this expected behavior? Do we want redirects to bubble up to the visit helper as a TransitionAborted error?
@chrisseto
Copy link

Possibly related emberjs/ember-test-helpers#332

@buschtoens
Copy link
Contributor Author

What I find weird, is that it used to work before. But yeah, possibly related. ☺️

@rwjblue
Copy link
Member

rwjblue commented Oct 25, 2018

Related to #17152...

@rwjblue rwjblue mentioned this issue Oct 25, 2018
Closed
@wagenet
Copy link
Member

wagenet commented Oct 25, 2018

Looks like the same as #17152.

See https://github.com/wagenet/ember-transition-abort-test which passes on latest and beta, but fails on canary, as visible here: https://travis-ci.com/wagenet/ember-transition-abort-test.

The commit that causes issues is 9b7e0f5, though on the face of it, it should be unobjectionable.

@rwjblue rwjblue mentioned this issue Oct 26, 2018
Merged
@rwjblue
Copy link
Member

rwjblue commented Oct 26, 2018

I submitted #17155 to revert 9b7e0f5 while we work through some timing issues.

@rwjblue
Copy link
Member

rwjblue commented Oct 29, 2018

Hmm, the fix above definitely addressed the reproduction that @wagenet shared but I'm not 100% sure it will fix the issue @buschtoens has. The commit that I reverted (9b7e0f5) doesn't exist in the current beta branch...

@buschtoens - Can you help us track this down with a reproduction?

@rwjblue rwjblue reopened this Oct 29, 2018
davidtaylorhq added a commit to davidtaylorhq/router.js that referenced this issue Nov 16, 2023
@davidtaylorhq
Fix followRedirects when source is async and destination is sync
7b0cb53 
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
@davidtaylorhq davidtaylorhq mentioned this issue Nov 16, 2023
Merged
davidtaylorhq added a commit to davidtaylorhq/router.js that referenced this issue Nov 16, 2023
@davidtaylorhq
Fix followRedirects when source is async and destination is sync
787cfbe 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Needs Reproduction Regression Router Bugs
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@wagenet @rwjblue @pzuraq @buschtoens @chrisseto