-
-
Notifications
You must be signed in to change notification settings - Fork 199
feat: Add processUrlSearchParams middleware as adapter prop #1110
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
Conversation
@Multiply is attempting to deploy a commit to the 47ng Team on Vercel. A member of the Team first needs to authorize it. |
9f01277
to
1d9f1f5
Compare
The latest updates on your projects. Learn more about Vercel for GitHub.
|
1d9f1f5
to
81da35a
Compare
There are some linting errors, is your Prettier not set to format on save? |
81da35a
to
ddbf7f4
Compare
I generally don't use prettier in my own projects, but I've run it now. 🚀 |
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! I'll add a section in the docs (we could also have a demo component for this 👀)
If you can give some pointers, I can add it when I get home, later tonight. |
I was thinking of reusing the same technique I used in the nuqs-2.5 blog post (for the key isolation demo): wrapping a part of the tree with the React SPA adapter, to scope the effect to search params updates happening in its children. Location-wise, it would probably be best to add a new heading to the options page ("Adapter props"), have the defaultOptions live in there too, and showcase the alphabetical sorting as both a code block for copy-paste and a demo. The demo could have three buttons updating counters in keys "a", "b", and "c", with a toggle to enable/disable the sorting (ie: switching the |
ddbf7f4
to
215bde1
Compare
@franky47 most likely not exactly as you requested, but I hope it's a good starting point. |
215bde1
to
135afcb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks! I'll do some minor wording tweaks and this should be good to merge.
Let me know if you need anything else. Happy to test the final changes in our project. |
Feel free to try the pkg.pr.new link in your app, I added reactivity for a change in the callback in useQueryStates, just need to find a clean way to test that. |
I don't have a need for the reactivity bit, but it definitely works as is, if it's static at least. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for processing URL search parameters through a middleware function. The feature allows users to provide a callback that modifies the URLSearchParams before they are sent to the adapter for URL updates.
- Introduces
processUrlSearchParams
prop to adapters for transforming search parameters - Updates the throttle queue system to accept and apply the processing function
- Adds comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
turbo.json | Increases build concurrency to 11 |
packages/nuqs/src/useQueryStates.ts | Integrates processUrlSearchParams from adapter context |
packages/nuqs/src/useQueryStates.test.ts | Adds tests for the new search params processing feature |
packages/nuqs/src/lib/queues/throttle.ts | Updates queue to accept and apply processUrlSearchParams callback |
packages/nuqs/src/lib/queues/throttle.test.ts | Adds unit tests for throttle queue processing functionality |
packages/nuqs/src/adapters/testing.ts | Adds processUrlSearchParams support to testing adapter |
packages/nuqs/src/adapters/lib/context.ts | Defines processUrlSearchParams in adapter context and exports hook |
packages/docs/content/docs/options.mdx | Documents the new processUrlSearchParams feature with examples |
packages/docs/content/docs/options.client.tsx | Provides interactive demos for the documentation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Thanks for your help! I have a few things I need to work on before this lands in 2.6.0 (likely by the end of the week). |
🎉 This PR is included in version 2.6.0-beta.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
processUrlSearchParams={(search) => { | ||
const entries = Array.from(search.entries()) | ||
entries.sort(([a], [b]) => a.localeCompare(b)) | ||
return new URLSearchParams(entries) | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was it considered to recommend the usage of the native sort()
function on URLSearchParams
?
Something along the lines of:
processUrlSearchParams={(search) => { | |
const entries = Array.from(search.entries()) | |
entries.sort(([a], [b]) => a.localeCompare(b)) | |
return new URLSearchParams(entries) | |
}} | |
processUrlSearchParams={(search) => { | |
const clone = new URLSearchParams(search) | |
clone.sort() | |
return clone | |
}} |
Whilst it isn't the exact same sorting algorithm, I was thinking maybe it'd be preferable to recommend following the standard by default!
I can update the docs if you agree 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't care about immutability, simply call search.sort()
and returning search
directly could also do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imagine building an entire library around URLSearchParams and only discovering the .sort function now 😅
Yes standards would be better to advertise in the docs. It's a shame it doesn't return itself though, that could have been a one-liner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closes #1109.