remix-run / react-router

Declarative routing for React
https://reactrouter.com
MIT License
52.77k stars 10.22k forks source link

[Docs]: Missing info for useSearchParams #11622

Open alesmenzel opened 3 months ago

alesmenzel commented 3 months ago

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.

kailash360 commented 1 month ago

@alesmenzel Can I work on this?

alesmenzel commented 1 month ago

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

yoganandaness commented 2 days ago

@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.