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

Remove inaccurate console warning for POP navigations #10030

Merged
merged 1 commit into from
Feb 2, 2023
Merged

Remove inaccurate console warning for POP navigations #10030

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/healthy-moons-compete.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"react-router": patch
"@remix-run/router": patch
---

Remove inaccurate console warning for POP navigations
10 changes: 6 additions & 4 deletions packages/react-router/lib/hooks.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -831,9 +831,7 @@ export function useAsyncError(): unknown {
return value?._error;
}

// useBlocker() is a singleton for now since we don't have any compelling use
// cases for multi-blocker yet
let blockerKey = "blocker-singleton";
let blockerId = 0;

/**
* Allow the application to block navigations within the SPA and present the
Expand All @@ -843,6 +841,7 @@ let blockerKey = "blocker-singleton";
*/
export function useBlocker(shouldBlock: boolean | BlockerFunction): Blocker {
let { router } = useDataRouterContext(DataRouterHook.UseBlocker);
let [blockerKey] = React.useState(() => String(++blockerId));

let blockerFunction = React.useCallback<BlockerFunction>(
(args) => {
Expand All @@ -856,7 +855,10 @@ export function useBlocker(shouldBlock: boolean | BlockerFunction): Blocker {
let blocker = router.getBlocker(blockerKey, blockerFunction);

// Cleanup on unmount
React.useEffect(() => () => router.deleteBlocker(blockerKey), [router]);
React.useEffect(
() => () => router.deleteBlocker(blockerKey),
[router, blockerKey]
);

return blocker;
}
Expand Down
36 changes: 12 additions & 24 deletions packages/router/router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -754,10 +754,6 @@ export function createRouter(init: RouterInit): Router {
// cancel active deferreds for eliminated routes.
let activeDeferreds = new Map<string, DeferredData>();

// We ony support a single active blocker at the moment since we don't have
// any compelling use cases for multi-blocker yet
let activeBlocker: string | null = null;

// Store blocker functions in a separate Map outside of router state since
// we don't need to update UI state if they change
let blockerFunctions = new Map<string, BlockerFunction>();
Expand All @@ -782,7 +778,7 @@ export function createRouter(init: RouterInit): Router {
}

warning(
activeBlocker != null && delta === null,
blockerFunctions.size === 0 || delta != null,
Comment on lines -785 to +781
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the incorrect boolean condition causing the warning, it should have been activeBlocker == null || delta != null since we only want to warn if we have a blocker and delta is null.

"You are trying to use a blocker on a POP navigation to a location " +
"that was not created by @remix-run/router. This will fail silently in " +
"production. This can happen if you are navigating outside the router " +
Expand Down Expand Up @@ -2123,12 +2119,6 @@ export function createRouter(init: RouterInit): Router {

if (blockerFunctions.get(key) !== fn) {
blockerFunctions.set(key, fn);
if (activeBlocker == null) {
// This is now the active blocker
activeBlocker = key;
} else if (key !== activeBlocker) {
warning(false, "A router only supports one blocker at a time");
}
Comment on lines -2126 to -2131
Copy link
Contributor Author

@brophdawg11 brophdawg11 Feb 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

While testing, I realized that this activeBlocker key storage was broken in strict mode since the effect cleanup runs after the second render. So we would get the following:

getBlocker('singleton-key');
getBlocker('singleton-key');
deleteBlocker('singleton-key');

Causing us to think we did not have a blocker. But that would get resolved as soon as we entered any form data causing the getBlocker to run again on the fn identity change.

Even changing to an incrementing key and tracking that wasn't sufficient since the effects cleanup cleans up the second render:

getBlocker('1');
getBlocker('2');
deleteBlocker('2');

Which also resulted in activeBlocker=null. See below for the new Map-only approach where we use a stack and the active blocker is just the top of the stack.

}

return blocker;
Expand All @@ -2137,9 +2127,6 @@ export function createRouter(init: RouterInit): Router {
function deleteBlocker(key: string) {
state.blockers.delete(key);
blockerFunctions.delete(key);
if (activeBlocker === key) {
activeBlocker = null;
}
}

// Utility function to update blockers, ensuring valid state transitions
Expand Down Expand Up @@ -2170,18 +2157,19 @@ export function createRouter(init: RouterInit): Router {
nextLocation: Location;
historyAction: HistoryAction;
}): string | undefined {
if (activeBlocker == null) {
if (blockerFunctions.size === 0) {
return;
}

// We only allow a single blocker at the moment. This will need to be
// updated if we enhance to support multiple blockers in the future
let blockerFunction = blockerFunctions.get(activeBlocker);
invariant(
blockerFunction,
"Could not find a function for the active blocker"
);
let blocker = state.blockers.get(activeBlocker);
// We ony support a single active blocker at the moment since we don't have
// any compelling use cases for multi-blocker yet
if (blockerFunctions.size > 1) {
warning(false, "A router only supports one blocker at a time");
}

let entries = Array.from(blockerFunctions.entries());
let [blockerKey, blockerFunction] = entries[entries.length - 1];
let blocker = state.blockers.get(blockerKey);
Comment on lines +2170 to +2172
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Active blocker is just the last (top) entry in the Map since entries are ordered by insertion time in a Map


if (blocker && blocker.state === "proceeding") {
// If the blocker is currently proceeding, we don't need to re-check
Expand All @@ -2192,7 +2180,7 @@ export function createRouter(init: RouterInit): Router {
// At this point, we know we're unblocked/blocked so we need to check the
// user-provided blocker function
if (blockerFunction({ currentLocation, nextLocation, historyAction })) {
return activeBlocker;
return blockerKey;
}
}

Expand Down