-
-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
Fix unstable_useBlocker key issues in strict mode #10573
Changes from all commits
e79325c
78039b4
46786a2
3f23e57
2fbff63
6c6c564
d8f42b7
adacb89
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
--- | ||
"react-router": patch | ||
"@remix-run/router": patch | ||
--- | ||
|
||
Fix `unstable_useBlocker` key issues in `StrictMode` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"react-router": patch | ||
--- | ||
|
||
Strip `basename` from locations provided to `unstable_useBlocker` functions to match `useLocation` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,8 @@ import { | |
matchRoutes, | ||
parsePath, | ||
resolveTo, | ||
stripBasename, | ||
IDLE_BLOCKER, | ||
UNSAFE_getPathContributingMatches as getPathContributingMatches, | ||
UNSAFE_warning as warning, | ||
} from "@remix-run/router"; | ||
|
@@ -933,30 +935,55 @@ let blockerId = 0; | |
* cross-origin navigations. | ||
*/ | ||
export function useBlocker(shouldBlock: boolean | BlockerFunction): Blocker { | ||
let { router } = useDataRouterContext(DataRouterHook.UseBlocker); | ||
let { router, basename } = useDataRouterContext(DataRouterHook.UseBlocker); | ||
let state = useDataRouterState(DataRouterStateHook.UseBlocker); | ||
let [blockerKey] = React.useState(() => String(++blockerId)); | ||
|
||
let [blockerKey, setBlockerKey] = React.useState(""); | ||
let [blocker, setBlocker] = React.useState<Blocker>(IDLE_BLOCKER); | ||
let blockerFunction = React.useCallback<BlockerFunction>( | ||
(args) => { | ||
return typeof shouldBlock === "function" | ||
? !!shouldBlock(args) | ||
: !!shouldBlock; | ||
(arg) => { | ||
if (typeof shouldBlock !== "function") { | ||
return !!shouldBlock; | ||
} | ||
if (basename === "/") { | ||
return shouldBlock(arg); | ||
} | ||
|
||
// If they provided us a function and we've got an active basename, strip | ||
// it from the locations we expose to the user to match the behavior of | ||
// useLocation | ||
let { currentLocation, nextLocation, historyAction } = arg; | ||
return shouldBlock({ | ||
currentLocation: { | ||
...currentLocation, | ||
pathname: | ||
stripBasename(currentLocation.pathname, basename) || | ||
currentLocation.pathname, | ||
}, | ||
nextLocation: { | ||
...nextLocation, | ||
pathname: | ||
stripBasename(nextLocation.pathname, basename) || | ||
nextLocation.pathname, | ||
}, | ||
historyAction, | ||
}); | ||
}, | ||
[shouldBlock] | ||
[basename, shouldBlock] | ||
); | ||
|
||
let blocker = router.getBlocker(blockerKey, blockerFunction); | ||
|
||
// Cleanup on unmount | ||
React.useEffect( | ||
() => () => router.deleteBlocker(blockerKey), | ||
[router, blockerKey] | ||
); | ||
React.useEffect(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this introduced a useEffect loop since setBlockerKey is used in the dependency array and changed in the same effect. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #10650 please have a look |
||
let key = String(++blockerId); | ||
setBlocker(router.getBlocker(key, blockerFunction)); | ||
setBlockerKey(key); | ||
return () => router.deleteBlocker(key); | ||
}, [router, setBlocker, setBlockerKey, blockerFunction]); | ||
|
||
// Prefer the blocker from state since DataRouterContext is memoized so this | ||
// ensures we update on blocker state updates | ||
return state.blockers.get(blockerKey) || blocker; | ||
return blockerKey && state.blockers.has(blockerKey) | ||
? state.blockers.get(blockerKey)! | ||
: blocker; | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -893,8 +893,9 @@ export function createRouter(init: RouterInit): Router { | |
init.history.go(delta); | ||
}, | ||
reset() { | ||
deleteBlocker(blockerKey!); | ||
updateState({ blockers: new Map(router.state.blockers) }); | ||
let blockers = new Map(state.blockers); | ||
blockers.set(blockerKey!, IDLE_BLOCKER); | ||
updateState({ blockers }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid mutating the |
||
}, | ||
}); | ||
return; | ||
|
@@ -991,9 +992,8 @@ export function createRouter(init: RouterInit): Router { | |
|
||
// On a successful navigation we can assume we got through all blockers | ||
// so we can start fresh | ||
for (let [key] of blockerFunctions) { | ||
deleteBlocker(key); | ||
} | ||
let blockers = new Map(); | ||
blockerFunctions.clear(); | ||
|
||
// Always respect the user flag. Otherwise don't reset on mutation | ||
// submission navigations unless they redirect | ||
|
@@ -1032,7 +1032,7 @@ export function createRouter(init: RouterInit): Router { | |
newState.matches || state.matches | ||
), | ||
preventScrollReset, | ||
blockers: new Map(state.blockers), | ||
blockers, | ||
}); | ||
|
||
// Reset stateful navigation vars | ||
|
@@ -1130,8 +1130,9 @@ export function createRouter(init: RouterInit): Router { | |
navigate(to, opts); | ||
}, | ||
reset() { | ||
deleteBlocker(blockerKey!); | ||
updateState({ blockers: new Map(state.blockers) }); | ||
let blockers = new Map(state.blockers); | ||
blockers.set(blockerKey!, IDLE_BLOCKER); | ||
updateState({ blockers }); | ||
}, | ||
}); | ||
return; | ||
|
@@ -2378,8 +2379,9 @@ export function createRouter(init: RouterInit): Router { | |
`Invalid blocker state transition: ${blocker.state} -> ${newBlocker.state}` | ||
); | ||
|
||
state.blockers.set(key, newBlocker); | ||
updateState({ blockers: new Map(state.blockers) }); | ||
let blockers = new Map(state.blockers); | ||
blockers.set(key, newBlocker); | ||
updateState({ blockers }); | ||
} | ||
|
||
function shouldBlockNavigation({ | ||
|
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 increment into a side effect to be compliant with
StrictMode
. Fetchers probably have a similar issue but they don't need to put anything like a blocker function into an internal router cache so it doesn't end up causing any user-facing issues.