-
-
Notifications
You must be signed in to change notification settings - Fork 10.6k
fix(react-router): fix useSearchParams
when used together w/ useBlocker
#12784
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
base: dev
Are you sure you want to change the base?
Conversation
…URLSearchParams instance
|
Hi @yuhwan-park, 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 [email protected]. Thanks! - The Remix team |
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
Hi Team! I'd love to hear your thoughts on my solution approach. I believe this issue could come up quite frequently in real-world scenarios. It's pretty common to implement navigation blocking using query strings and useBlocker (e.g., ?mode=edit). From my perspective, the changes I proposed don't affect the React rendering cycle or useSearchParams hook original behavior at all - they just fix a bug related to object reference equality. However, I want to make sure this approach isn't too aggressive or radical. What do you think? |
useSearchParams
when used together w/ useBlocker
typeof nextInit === "function" | ||
? nextInit(new URLSearchParams(searchParams)) | ||
: nextInit |
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.
Not sure if we want to add the URLSearchParams
call here or in getSearchParamsForLocation
🤔
CC/ @brophdawg11
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.
The getSearchParamsForLocation
function already returns a new URLSearchParams
object each time.
Here's what's happening in the flow that causes the bug:
setSearchParams
executes (this is wheresearchParams
changes)
setSearchParams((prev) => {
prev.set("mode", "edit");
return prev;
})
router.navigate()
runs- It gets blocked by
useBlocker
- Even though the
location.pathname
hasn't changed, thesearchParams
returned byuseSearchParams
remains in the modified state
Because of this flow, I think we need to create a deep copy of the URLSearchParams
before passing it to the nextInit
function - regardless of how nextInit
is implemented - to ensure it behaves as intended.
Issue Overview
nextInit
, if thenextInit
implementation modifies properties using methods likedelete()
oradd()
. the internal property values ofsearchParams
change before thenavigate
function executes.useBlocker
hook, this can lead tosearchParams
values that don't match withlocation.search
(which is a dependency).Solution
searchParams
. This ensures that even ifsearchParams
values are modified in thenextInit
function, thesearchParams
within the hook remains unchanged.I have confirmed that issue #12256 has been resolved by implementing the exact solution specified in the issue description.
closes: #12256 #12188