-
-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Remove inaccurate console warning for POP navigations #10030
Conversation
🦋 Changeset detectedLatest commit: 4f1ef58 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 |
activeBlocker != null && delta === null, | ||
blockerFunctions.size === 0 || delta != null, |
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 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
.
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"); | ||
} |
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.
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.
let entries = Array.from(blockerFunctions.entries()); | ||
let [blockerKey, blockerFunction] = entries[entries.length - 1]; | ||
let blocker = state.blockers.get(blockerKey); |
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.
Active blocker is just the last (top) entry in the Map since entries are ordered by insertion time in a Map
Fixes an invalid boolean condition on our
warning
and also removes theactiveBlocker
internal state variable since it was not actually correct in strict mode.Closes #9998