fix: Wait for restore url navigation to complete before proceeding #11620
Conversation
🦋 Changeset detectedLatest commit: 6071fb1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Hi @Artur-, Welcome, and thank you for contributing to React Router! Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once. You may review the CLA and sign it by adding your name to contributors.yml. Once the CLA is signed, the If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run. Thanks! - The Remix team |
|
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
This comment was marked as outdated.
This comment was marked as outdated.
Artur-
force-pushed
the
wait-for-restore-url
branch
from
4968e21
to
412a12e
Compare
Artur-
force-pushed
the
wait-for-restore-url
branch
from
15781aa
to
1682389
Compare
|
Any chance of getting this reviewed and into v6? |
Artur-
force-pushed
the
wait-for-restore-url
branch
from
c4745de
to
1682389
Compare
Artur-
force-pushed
the
wait-for-restore-url
branch
from
55fa06c
to
82d4344
Compare
|
The test in 82d4344 is for this case and fails in the |
|
@markdalgleish any comment on this? Is there a way to get this moving forward instead of collecting dust and conflicts? |
| reset: undefined, | ||
| location: undefined, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Could we revert these unrelated changes to the tests and do them in a standalone PR? I'm not against changing the approach but I'd prefer to keep the functional bug fix changes separate from the unrelated test cleanup changes. Bug fix PRs like this are, IMO, best when they are 1 or more net-new tests demonstrating the bug and the code changes to make those test pass
There was a problem hiding this comment.
Yes, I really do not want to make any changes to any other tests but this was needed, at least originally, to get tests to pass with the fix. I now reverted all these changes and included one commit with the test that fails in current dev and another commit with the fix. Let's see if tests pass or not, and if not, I can create a separate PR first with the test changes.
There was a problem hiding this comment.
ok let me take a look at those locally. I noticed this was off dev too but we'll need it to be off v6 if we want to release a fix in 6.x so I'll probably re-open a new PR against v6 and will include you as a coauthor on that one
| @@ -1007,7 +1007,7 @@ export function createRouter(init: RouterInit): Router { | |||
|
|
|||
| // Flag to ignore the next history update, so we can revert the URL change on | |||
| // a POP navigation that was blocked by the user without touching router state | |||
| let ignoreNextHistoryUpdate = false; | |||
| let ignoreNextHistoryUpdate: (() => void) | undefined = undefined; | |||
There was a problem hiding this comment.
Could we rename this to something like unblockBlockerHistoryUpdate to align with the new approach?
| ); // This awaits the navigation | ||
| router.getBlocker("KEY", fn).proceed!(); | ||
| await waitFor(() => | ||
| expect(router.state.location.pathname).toBe("/about") |
There was a problem hiding this comment.
Are we sure this test exhibits the actual bug? I see this failure when run against dev:
● navigation blocking › proceeds from blocked state using browser history › proceeds after quick block of back navigation
expect(received).toBe(expected) // Object.is equality
Expected: "/about"
Received: "/"
The reproduction for the original issue leaves you on the original blocked page (/three) but this test, without the fix, actually ends up going back 2 history locations?
There was a problem hiding this comment.
I am not sure why it behaves differently here. My assumption was that maybe JSDOM (or whoever emulates browser navigation here) behaves differently in the case where navigation is in progress and you navigate again. I think it still covers the issue though, which is that if you navigate during navigation, you will end up somewhere else than you would expect.
If you have ideas on how to improve the test, I am all ears
There was a problem hiding this comment.
Ah ok yeah this is probably a JSDOM issue. I'm going to test this through an integration test in a real browser in remix-run/remix#9914 instead of trying to hack JSDOM into behaving correctly
|
Superseded by #11930 |
Fixes #11613