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.

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

[fixed] Abort pending transition if user navigates away #480

Merged
merged 3 commits into from
Nov 27, 2014

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Nov 18, 2014

Previously, navigating (by clicking Back button or a link) while a transition in progress would put router into an inconsistent state.

Now, a pending transition will be aborted by the router if user navigates away while it was pending.

Fixes #479

@gaearon gaearon changed the title [fixed] Pending transition is aborted if user navigates away [fixed] Abort pending transition if user navigates away Nov 18, 2014
@gaearon gaearon force-pushed the fix-transition-races branch from 5ca4f7a to df51721 Compare November 18, 2014 01:36
@gaearon gaearon force-pushed the fix-transition-races branch 2 times, most recently from bbc772b to 49ba0bc Compare November 26, 2014 19:48
@gaearon
Copy link
Contributor Author

gaearon commented Nov 26, 2014

I haven't written test cases for this, here's what I intended to do.
(Will need to test each case again, as I may have broken something midway through it.)

A -> slowly resolving B

  • Go. Path will change to /b but it's not ready yet. Now press Back.

Before fix: when B is ready, will navigate from A to B.
After fix: when B is ready, will stay at A because user undecided [sic] going to B.

  • Go. Path will change to /b but it's not ready yet. Now press link to C.

Before fix: when B is ready, will navigate from C to B.
After fix: when B is ready, nothing happens because user wanted C. If you press Back now, you will land at A.

  • Go. Path will change to /b but it's not ready yet. Now press Back twice.

Before fix: when B is ready, will navigate from A to B.
After fix: when B is ready, will stay at whatever came before A.

A -> slowly resolving and then aborting B

  • Go. Path will change to /b but it's not ready yet. Now press Back.

Before fix: when B is ready and aborted, will pop from A to whatever came before A.
After fix: when B is ready and aborted, will stay at A because user wanted to pop B and not A.

  • Go. Path will change to /b but it's not ready yet. Now press link to C.

Before fix: when B is ready and aborted, will pop from C to A.
After fix: when B is ready and aborted, nothing happens because user wanted C. If you press Back now, you will land at A.

  • Go. Path will change to /b but it's not ready yet. Now press Back twice.

Before fix: when B is ready and aborted, will pop yet further!
After fix: when B is ready and aborted, will stay at whatever came before A.

A -> slowly resolving and then redirecting B

Very similar to first case.

@gaearon gaearon force-pushed the fix-transition-races branch from 49ba0bc to 14bf154 Compare November 26, 2014 20:04
@gaearon
Copy link
Contributor Author

gaearon commented Nov 26, 2014

@rpflorence @mjackson

I’m afraid I don’t have time to write tests for this today, but I verified those scenarios I describe and they work. We need to see if all react-router examples work correctly, and if they do, I think it’s still better to bring this in without tests because previous racing behavior was broken anyway.

@gaearon gaearon force-pushed the fix-transition-races branch 2 times, most recently from 5508b75 to 888b03a Compare November 26, 2014 20:32
@ryanflorence
Copy link
Member

f1c84b8

@gaearon
Copy link
Contributor Author

gaearon commented Nov 26, 2014

@gaearon gaearon force-pushed the fix-transition-races branch from 888b03a to 566e8cf Compare November 26, 2014 21:55
Previously, navigating (by clicking Back button or a link) while a transition in progress would put router into an inconsistent state.
Now, a pending transition will be aborted and ignored by the router if user navigates away while it was pending.

Fixes remix-run#479
@gaearon gaearon force-pushed the fix-transition-races branch from 566e8cf to 91d4380 Compare November 26, 2014 21:56
@gaearon
Copy link
Contributor Author

gaearon commented Nov 26, 2014

@mjackson

I cleaned it up, would you take a look?

I'll be adding tests for now..

@gaearon
Copy link
Contributor Author

gaearon commented Nov 26, 2014

Now that I'm writing tests, I'll write tests for abort as well (they're missing).

@gaearon
Copy link
Contributor Author

gaearon commented Nov 26, 2014

Added tests for basic async transitions (not not the corner cases I'm describing here yet).
Will push corner cases in a couple of hours.

@gaearon gaearon force-pushed the fix-transition-races branch from a3c7b93 to d16d658 Compare November 26, 2014 23:20
@mjackson
Copy link
Member

@gaearon This LGTM so far. Thank you for writing tests as well!

@gaearon
Copy link
Contributor Author

gaearon commented Nov 27, 2014

I added more tests. They cover scenarios I mentioned above.

done();
}, Async.delay / 2 + 10);
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have one more step here that basically just asserts "Foo" is still rendered after the delay and calls done?

@mjackson
Copy link
Member

Thanks @gaearon ! This looks great. Hopefully I made some useful comments.

@gaearon gaearon force-pushed the fix-transition-races branch from ea343ec to 9f7df0b Compare November 27, 2014 01:53
mjackson added a commit that referenced this pull request Nov 27, 2014
[fixed] Abort pending transition if user navigates away
@mjackson mjackson merged commit b6ef894 into remix-run:master Nov 27, 2014
@gaearon gaearon deleted the fix-transition-races branch November 27, 2014 01:57
@ryanflorence
Copy link
Member

@gaearon, this project is way better because of you, thanks :)

@gaearon
Copy link
Contributor Author

gaearon commented Nov 27, 2014

@rpflorence I'm glad to be of use!

@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle races between user actions and transitions
3 participants