-
-
Notifications
You must be signed in to change notification settings - Fork 10.6k
@remix-run/router
: Add support for navigation blocking
#9709
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
🦋 Changeset detectedLatest commit: 7749172 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
/.env | ||
/NOTES.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed somewhere to keep notes because this was getting confusing 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
return ( | ||
<> | ||
<p style={{ color: "red" }}> | ||
Blocked the last navigation to {blocker.location.pathname} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this to a separate component so this wouldn't blow up on blocker.location.pathname
when in an unblocked state. everything got evaluated in the above key lookup approach.
packages/router/history.ts
Outdated
if (listener) { | ||
index = nextIndex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should always be updating this even if no one is listening - never showed up as an issue since we always have a listener
packages/router/router.ts
Outdated
// Short circuit if navigation is blocked | ||
if ( | ||
Array.from(state.blockers).some(([_, blocker]) => { | ||
return blocker.state === "blocked"; | ||
}) | ||
) { | ||
return; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think his is leftover form very early. Our entry points are popstate
and router.navigate
so we can just check for blocking there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// case we should log a warning as it will result in bugs. | ||
if (index == null) { | ||
index = 0; | ||
globalHistory.replaceState({ ...globalHistory.state, idx: index }, ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This replaceState()
line is causing a new regression for my project when testing with Jest and JSDom. I observed the following series of circumstances:
-
The test runs in React strict mode, causing the React Router to mount twice.
-
When the React Router is loaded the 1st time,
window.location.href
ishttp://www.example.com/path/to/route
-
When the React Router is loaded the 1st time,
index
isnull
. -
The
replaceState
changeswindow.location.href
tohttp://localhost/
. Notice both the origin and the path has changed. -
When the React Router is loaded the 2nd time (strict mode), the URL is now wrong, causing the wrong route to load and my test to fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose the following which fixes my problem: #10001
This PR adds support for low-level blocking of navigations within the app's location origin via
unstable_useBlocker
. We are confident in this approach but wanted to initially release it under a flag to give users an opportunity to test and provide feedback.In addition, I added an options parameter to
useBeforeUnload
so that users can have more control over how that event listener behaves when dealing with cross-origin navigations.Closes #8139.