Skip to content

fix: make sure the state-change effect runs for multi parsers#1163

Merged
franky47 merged 5 commits into47ng:nextfrom
TkDodo:feature/1162-multi-parsers-back-button
Oct 6, 2025
Merged

fix: make sure the state-change effect runs for multi parsers#1163
franky47 merged 5 commits into47ng:nextfrom
TkDodo:feature/1162-multi-parsers-back-button

Conversation

@TkDodo
Copy link
Contributor

@TkDodo TkDodo commented Oct 6, 2025

fixes #1162

the dependency array of the state-change effect included stringified versions of searchParams, but it was always using .get, which was only seeing the first search param.

With multi-parsers, we need to look at all searchParams. It should be safe here to always call .getAll(), because even for one key, this would be stable.

@vercel
Copy link

vercel bot commented Oct 6, 2025

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

A member of the Team first needs to authorize it.

Comment on lines -156 to +164
Object.values(resolvedUrlKeys).map(urlKey => [
urlKey,
initialSearchParams?.get(urlKey) ?? null
])
Object.entries(resolvedUrlKeys).map(([key, urlKey]) => {
const parser = keyMap[key]
return [
urlKey,
parser?.type === 'multi'
? initialSearchParams?.getAll(urlKey)
: (initialSearchParams?.get(urlKey) ?? null)
]
})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here, I’m trying to address a caching issue where we were putting the wrong values into the cachedQuery ( = queryRef.current) cache for multi parsers, because we were only reading the first search param with .get

Now, we are reading with .getAll for multi-parsers

@franky47 I’m not really sure how / where to add a test for this. I did add an e2e test for issue #1162

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the e2e test should be enough for this, thanks!

@TkDodo TkDodo marked this pull request as ready for review October 6, 2025 13:40
@franky47 franky47 added this to the 🪵 Backlog milestone Oct 6, 2025
Comment on lines -156 to +164
Object.values(resolvedUrlKeys).map(urlKey => [
urlKey,
initialSearchParams?.get(urlKey) ?? null
])
Object.entries(resolvedUrlKeys).map(([key, urlKey]) => {
const parser = keyMap[key]
return [
urlKey,
parser?.type === 'multi'
? initialSearchParams?.getAll(urlKey)
: (initialSearchParams?.get(urlKey) ?? null)
]
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the e2e test should be enough for this, thanks!

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

pkg-pr-new bot commented Oct 6, 2025

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

commit: 7222c22

@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 1:51pm

@franky47 franky47 merged commit 9eb3e2d into 47ng:next Oct 6, 2025
29 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/1162-multi-parsers-back-button 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When navigating using forward/back 'parseAsNativeArrayOf' does not trigger state-change

2 participants