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

style: Use useSyncExternalStore in the React adapter #723

Draft
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

bartlangelaan
Copy link

@bartlangelaan bartlangelaan commented Oct 30, 2024

This utilizes the useSyncExternalStore hook in the React adapter, which automatically takes care of rehydration errors.

I'm unsure if it still works correctly, because this version does not use the emitter value anymore. It just gets it from the location every time. I will test it before marking it as ready.

Copy link

vercel bot commented Oct 30, 2024

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

A member of the Team first needs to authorize it.

@bartlangelaan bartlangelaan marked this pull request as draft October 30, 2024 05:47
@franky47
Copy link
Member

Thanks, wow this is so much cleaner!

I had heard of useSyncExternalStore, but it didn't occur to me to use it here, it sounds like a perfect use-case. Great job!

Copy link

vercel bot commented Oct 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nuqs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 30, 2024 10:57pm

@franky47 franky47 added the deploy:preview Deploy a preview version of this PR on pkg.pr.new label Oct 30, 2024
@bartlangelaan
Copy link
Author

I don't have time to test it today, so if you can't wait and want to test it, feel free! :-)

@franky47
Copy link
Member

Regarding the emitter, it's use is to make sure the searchParams returned by the adapter hook is reactive: when the URL changes, no matter the reason (internal query state change, Link, back/fwd navigation), every hook needs to get the new value.

@franky47 franky47 added deploy:preview Deploy a preview version of this PR on pkg.pr.new and removed deploy:preview Deploy a preview version of this PR on pkg.pr.new labels Oct 30, 2024
Copy link

pkg-pr-new bot commented Oct 30, 2024

Open in Stackblitz

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

commit: 6b2e0a2

@franky47
Copy link
Member

Hey, do you have some bandwidth to work on this again, or do you mind if I pick it up?

@bartlangelaan
Copy link
Author

I don't mind at all! I don't know when I will have time for this, so feel free!

@franky47 franky47 added this to the 🪵 Backlog milestone Nov 13, 2024

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no suggestions.

Comments skipped due to low confidence (1)

packages/nuqs/src/adapters/react.ts:7

  • Changing the emitter type from { update: URLSearchParams } to { update: void } might cause issues if other parts of the codebase rely on the update event carrying a URLSearchParams payload.
const emitter = mitt<{ update: void }>()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adapters/react Uses the React SPA adapter deploy:preview Deploy a preview version of this PR on pkg.pr.new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants