Skip to content

Conversation

@TkDodo
Copy link
Contributor

@TkDodo TkDodo commented Oct 4, 2025

When useQueryStates receives an event that should update its internal state. we should bail out of that update if we already store the same value.

This is what React’s useState does per default (by comparing with Object.is), however, it doesn’t work to delegate this to React because the internal state of useQueryStates is an object. When an event comes in, we only update a single value of that object:

stateRef.current = {
  ...stateRef.current,
  [stateKey as keyof KeyMap]: state ?? defaultValue ?? null
}

so we can optimize ourselves by comparing the current state for that key with the new state that comes in from the emit and skip calling setInternalState for those cases.


This perf fix will be helpful for the writeDefaults feature:

because writing the default value to the url upon rendering should not trigger another render, as the states are already equal.

@vercel
Copy link

vercel bot commented Oct 4, 2025

@TkDodo is attempting to deploy a commit to the 47ng Team on Vercel.

A member of the Team first needs to authorize it.

@franky47 franky47 added the deploy:preview Deploy a preview version of this PR on pkg.pr.new label Oct 5, 2025
@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 5, 2025

pnpm add https://pkg.pr.new/nuqs@1160

commit: 5976fb4

@franky47
Copy link
Member

franky47 commented Oct 5, 2025

Could this be a solution to #924 (comment) ?

@TkDodo
Copy link
Contributor Author

TkDodo commented Oct 5, 2025

Could this be a solution to #924 (comment) ?

Hm, I don't think so. The bailout happens later, only for setting component state. And with clearOnDefault: false, we might actually need to write to the url even though we set to the same value in state if it's equal to the default.

@TkDodo TkDodo marked this pull request as ready for review October 6, 2025 08:01
@TkDodo
Copy link
Contributor Author

TkDodo commented Oct 6, 2025

@franky47 do you know what I can do against the react-router v7 test failure? I think this is a shared test, but it seems that we have one fewer re-renders now (which is a good thing)

@franky47
Copy link
Member

franky47 commented Oct 6, 2025

I'll run it again, the render test can be flaky sometimes, I don't like it but it has been useful in detecting major re-render changes in the past.

@TkDodo
Copy link
Contributor Author

TkDodo commented Oct 6, 2025

I also need to add another test that calls setState(v => v) to confirm the behaviour compared to main

@franky47 franky47 added this to the 🪵 Backlog milestone Oct 6, 2025
@franky47
Copy link
Member

franky47 commented Oct 6, 2025

The one change I see that could happen here is that a setState(v => v, { shallow: false }) could have been used before to refresh server-side data, but there might be better (more explicit) ways to do this in userland.

@TkDodo
Copy link
Contributor Author

TkDodo commented Oct 6, 2025

I actually messed up the last refactoring and the test showed it 🎉

Should be good now: f17fe59

@vercel
Copy link

vercel bot commented Oct 6, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
nuqs Ready Ready Preview Comment Oct 6, 2025 2:21pm

@franky47
Copy link
Member

franky47 commented Oct 6, 2025

I think there was a place in the docs or the playground (need to dig) where I explored a pattern to collect state changes locally, without updating the URL (to avoid polluting the global history), and submit all in one go by using this setState(v => v) trick.

To be fair, the throttleMs: Infinity trick to disengage the URL update is likely to cause some more issues with writeDefaults, and the pattern described above is a form, really. #UseThePlatform

@TkDodo
Copy link
Contributor Author

TkDodo commented Oct 6, 2025

hmm I 'm not seeing that anywhere 🤔

@franky47
Copy link
Member

franky47 commented Oct 6, 2025

Found it, it's the deferred test in the e2e-next environment (which could be a shared one), and it still passes apparently:
https://github.com/47ng/nuqs/blob/next/packages/e2e/next/src/app/app/deferred/page.tsx

@franky47 franky47 merged commit 09a8f8a into 47ng:next Oct 6, 2025
30 checks passed
@github-actions
Copy link

github-actions bot commented Oct 6, 2025

🎉 This PR is included in version 2.7.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@franky47 franky47 mentioned this pull request Oct 6, 2025
@TkDodo TkDodo deleted the feature/setState-bail-out branch October 7, 2025 11:07
@franky47 franky47 removed this from the 🚀 Shipping next milestone Oct 14, 2025
I-3B pushed a commit to I-3B/nuqs that referenced this pull request Oct 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deploy:preview Deploy a preview version of this PR on pkg.pr.new feature/useQueryStates released

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants