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

useQueryStates updater function has an "old" value #763

Closed
geemanjs opened this issue Nov 13, 2024 · 6 comments · Fixed by #776
Closed

useQueryStates updater function has an "old" value #763

geemanjs opened this issue Nov 13, 2024 · 6 comments · Fixed by #776
Labels
adapters/remix Uses the Remix adapter bug Something isn't working feature/useQueryStates released

Comments

@geemanjs
Copy link

geemanjs commented Nov 13, 2024

Context

What's your version of nuqs?

2.1.2

What framework are you using?

  • ✅ Remix

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.

@geemanjs geemanjs added the bug Something isn't working label Nov 13, 2024
@franky47 franky47 added the adapters/remix Uses the Remix adapter label Nov 13, 2024
@franky47
Copy link
Member

franky47 commented Nov 13, 2024

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.

@franky47 franky47 added this to the 🪵 Backlog milestone Nov 13, 2024
@geemanjs
Copy link
Author

geemanjs commented Nov 14, 2024

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
Copy link
Member

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:

remix-rate-limits.mov

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.

@franky47
Copy link
Member

franky47 commented Nov 19, 2024

The original issue (stale value after navigation) could be solved by #776, could you try this and let me know if it fixes the issue on your end please? It does solve the issue in your reproduction repository.

npm i https://pkg.pr.new/nuqs@776

Copy link

🎉 This issue has been resolved in version 2.2.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@franky47 franky47 removed this from the 🚀 Shipping next milestone Nov 20, 2024
@geemanjs
Copy link
Author

Thanks @franky47 for changes in #776 - I have tested the changes out in our application and am pleased to report they work well 😄 ! Keep up the great work

James

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adapters/remix Uses the Remix adapter bug Something isn't working feature/useQueryStates released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants