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.

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

[Docs]: Missing info for useSearchParams #11622

Open
alesmenzel opened this issue Jun 6, 2024 · 3 comments
Open

[Docs]: Missing info for useSearchParams #11622

alesmenzel opened this issue Jun 6, 2024 · 3 comments
Labels

Comments

@alesmenzel
Copy link

Describe what's incorrect/missing in the documentation

It would be beneficial to mention in the docs whether user of react-router can mutate the previous searchParams or they should create a new instance.

https://github.com/remix-run/react-router/blob/5d66dbdbc8edf1d9c3a4d9c9d84073d046b5785b/packages/react-router-dom/index.tsx#L1467C7-L1467C33

const [searchParams, setSearchParams] = useSearchParams()

setSearchParams(searchParams => {
  searchParams.set('test', 'test')
  return searchParams
})

// versus

setSearchParams(searchParams => {
  const updated = new URLSearchParams(searchParams)
  updated.set('test', 'test')
  return updated
})

Since react-router does create a new URLSearchParams which is memorized by location.search https://github.com/remix-run/react-router/blob/main/packages/react-router-dom/dom.ts#L79, it seems it should be fine to just mutate the search params instead of creating new URLSearchParams. Can you please specify this in the docs.

@alesmenzel alesmenzel added the docs label Jun 6, 2024
@kailash360
Copy link

@alesmenzel
Can I work on this?

@alesmenzel
Copy link
Author

Hi @kailash360, feel free to work on this issue, but I think the maintainers should first give us their opinion though.

@yoganandaness
Copy link

@alesmenzel I think there is a real difference between the two.

setSearchParams(searchParams => { searchParams.set('test', 'test') return searchParams })
This mutates the searchParams directly, since set is a mutable function. As a result of this, before even returning the searchParams, the searchParams gets set with new value. This will result in the variable getting set before even the url changes takes place.

setSearchParams(searchParams => { const updated = new URLSearchParams(searchParams) updated.set('test', 'test') return updated })

This will not do any harm to the searchParams, since this was cloned into the variable updated.

This will really affect in a scenario where we do window.confirm. When clicking No, the url and searchParams variable should not change, but here the variable changes does when following the approach 1. Approach 2 is good, but cloning the searchParams is a super cumbersome task.

Do you agree? Correct me if i am wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants