47ng / nuqs

Type-safe search params state manager for React frameworks - Like useState, but stored in the URL query string.
https://nuqs.47ng.com
MIT License
4.76k stars 104 forks source link

useQueryStates updater function has an "old" value #763

Open geemanjs opened 4 days ago

geemanjs commented 4 days ago

Context

What's your version of nuqs?

2.1.2

What framework are you using?

Which version of your framework are you using?

2.14.0

Description

When using the "state updater" function the "previous" value is out of date.

In the example the search params are updated using remix's Link component (which is a standard <a href="?tab=b"> when rendered)

Reproduction

https://github.com/geemanjs/nuqs-remix-query-state-reproduction

Example: Steps to reproduce the behavior:

  1. Click a letter B in the top row - these update the url using the Link
  2. Click a number below 3 - these update the url with an updater function
  3. Watch how letter returns back to A (the default) when you click the number

In the example if you use nuqsState instead of the updater function it works correctly. I'm guessing a reference somewhere isn't updated.

franky47 commented 4 days ago

Thanks for the report, there's indeed a cached value in there that failed to follow the link update. I'll see what I can do tomorrow.

Interestingly, when clicking a link, I see two renders: one with the previous tab value, and one with the final one. Is that a Remix useNavigate behaviour specifically (not super familiar with it yet) ?

As an aside, you don't need to spread the previous value to update search params: only specify the ones you want to update, and they (should) be merged with the existing ones, so:

onClick={() => {
  setNuqsState((prev) => ({ ...prev, filters: '2' }))
}}

can usually be simplified to:

onClick={() => setNuqsState({ filters: '2' })}

Now with this bug, there is still something that sticks that shouldn't, but this is what should be working eventually.

geemanjs commented 3 days ago

Thanks for replying - I really like the api of this library!

Just looking through the code for the remix adapter I don't think you need to use useNavigate at all - the setSearchParams accepts a URLSearchParams object and the same options as useNavigate. It does lose the custom renderQueryString - but I suspect remix does something similar internally.

function useNuqsRemixAdapter() {
  const [searchParams, setSearchParams] = useSearchParams()

  const updateUrl = (search: URLSearchParams, options: AdapterOptions) => {
    setSearchParams(search, {
      replace: options.history === 'replace',
      preventScrollReset: !options.scroll
    })
  };

  return {
    searchParams,
    updateUrl
  }
}

I did have a quick look though and the double render still seems to remain with that change.

I wander if its because of how the library works useQueryState/useQueryStates

  1. It updates the local (client) state first [RENDER 1]
  2. It then queues to apply that state change to the url
  3. After the "throttling period" it applies the state change to the url
  4. The url change triggers a new pass of useSearchParams which in turn triggers [RENDER 2]

^^ I guess the only way around that would be to rely solely on the remix for the url state changes i.e. no throttling / queuing etc (I suspect that built into remix already) - but that ruins the model of a common "implementation" used across "multiple frameworks" with an adapter pattern - which is super clean.

Arguably remix overlaps quite a lot of what the library is aiming to achieve with the useSearchParams hook - but there is no way to pull out nicely typed search params in remix.

Something like this would perhaps be a more remix way... That would also get rid of the need for the high level context but I totally get its perhaps not the direction you want the library to go

// NOTE - I haven't tested this - Its just an off the cuff what I think it might look like
function useQueryState(key, parser) {
  const [params, setParams] = useSearchParams()
  const setSearchParam = (nextValue) => {
    const nextParams = new URLSearchParams(params)
    nextParams.set(key, parser.stringify(nextValue))
    setParams(nextParams, parser.options)
  }
  return [parser.parse(params.get(key)), setSearchParam]
}

const [state, setState] = useQueryState('count', parseAsInteger.withDefault(0).withOptions({
    history: 'push',
    shallow: true
  })
)
franky47 commented 3 days ago

Regarding the rate-limiting, Remix uses the History API directly, and so is subject to the same issue of hitting errors when updating state too quickly:

https://github.com/user-attachments/assets/7b131932-e585-4537-9ffc-f0d28ac2a8cf

The double render isn't surprising, as you said it's how nuqs works (optimistic updates, one for the local state and one when the URL has finished updating). It's the fact that the first render has the old value that seemed wrong, but it could be that I'm logging before it actually gets updated.

Regarding encoding, Remix uses the same platform standards as the others: URLSearchParams.toString, which calls encodeURIComponent on each value. The reason why we chose to bypass that is a hunch that browsers and modern URL transport mechanisms (email, social media, anywhere you can paste one and it becoming clickable) do accept more unencoded ASCII characters than they used to, and encodeURIComponent is probably too eager in encoding them all. See #372.