-
-
Notifications
You must be signed in to change notification settings - Fork 220
feat: multi-parsers #1134
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
feat: multi-parsers #1134
Conversation
|
@TkDodo is attempting to deploy a commit to the 47ng Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
|
Yeah I agree, feel free to bump it high enough (in nuqs' package.json) for it to pass and I'll see how to move it cleanly to another step (the docs build needs the actual size to display it on the landing page). |
|
I’ve increased the relevant size-limit for now a bit: |
that was probably premature, as useQueryStates needs something different
packages/nuqs/src/useQueryStates.ts
Outdated
| // todo this === comparison likely won't work with arrays | ||
| if (cachedQuery && cachedState && (cachedQuery[urlKey] ?? null) === query) { |
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.
cachedQueries now contain Iterable<string>, so we can’t really compare them with triple equals.
I’m wondering why the eq method of the parser isn’t used for comparison here?
But, we could also extract this “shallow equals” array comparison to a shared util:
nuqs/packages/nuqs/src/parsers.ts
Lines 484 to 492 in e8af136
| eq(a, b) { | |
| if (a === b) { | |
| return true // Referentially stable | |
| } | |
| if (a.length !== b.length) { | |
| return false | |
| } | |
| return a.every((value, index) => itemEq(value, b[index]!)) | |
| } |
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.
I’m wondering why the
eqmethod of the parser isn’t used for comparison here?
We're comparing serialised versions here (the eq function compares state types), so yeah we'd likely have to branch for string comparison with === vs array comparison for iterables.
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.
I looked into this a bit more, but I’m a bit confused which types get stored where:
const query =
queuedQuery === undefined
? parser.type === 'multi'
? (searchParams?.getAll(urlKey) ?? [])
: (searchParams?.get(urlKey) ?? null)
: queuedQuery
now when we read the Query from the searchParams, it can only be string | null | Array<string> - I’ve extracted that to its own type:
export type SearchParamQuery = string | Array<string>
but when I look at queuedQuery, it seems that we store whatever parser.serialize has returned, which used to only be strings, but now it can be Iterable<string>, so I’ve also made type for that:
export type SerializedQuery = string | Iterable<string>
now when trying to compare the two:
compareQuery(cachedQuery[urlKey] ?? null, query)
cachedQuery contains a SerializedQuery, and query can contain that too when we get it from queuedQuery, but if we read it directly from the searchParams, it will be a SearchParamQuery.
I don’t see how we could effectively compare the result of searchParams.getAll() with the result of a serialized Query, so maybe we need to run that trough parser.serialize before?
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.
I see, if .getAll() returns a Array<string>, that is already an Iterable<string>, so maybe we could have a comparison function dedicated to serialised representations (either read from the URL or serialised before an update)
so maybe we need to run that trough
parser.serializebefore?
The input of serialize being a state ("hydrated"/deserialised) data type, that wouldn't work, what we get from searchParams.get{,All}() is already in a serialised form.
The issue might be from the ?? null part in cachedQuery[urlKey] ?? null, as this makes sense for a single parser, but not much for a multi one (where we'd default to an empty array).
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.
so maybe we could have a comparison function dedicated to serialised representations (either read from the URL or serialised before an update)
yeah can try that.
The issue might be from the ?? null
does it even make sense to call the comparison function if cachedQuery[urlKey] doesn’t exist? this would never yield a cache hit imo...
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.
untested, but here is an attempt at that:
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.
One thing to look out for with cached queries is that null and undefined have different meanings: undefined means there is no cache in memory, but null here means we requested to remove that key from the URL when setting the state.
If you have suggestions for a better internal way of encoding these intents, I'm very much open to suggestions (maybe symbols?)
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.
yeah I kept that intent the same now (I think), and only fallback to ?? defaultValue instead of ?? null, where defaultValue = parser.type === 'multi' ? [] : null
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.
but note that ?? will use the fallback for both null and undefined, so I’m not sure if that differentiation between null and undefined was there before in all places 🤔
|
As I was trying to add some end-to-end tests, I’m stumbling over some issues I’m not quite sure how to fix / debug:
I can see that
|
otherwise, the default is never applied
I agree, in a lot of cases |
…erator into "write"
parsers don't return the default value, they return null (defaultValue gets applied later)
|
react-router v7 seems to fail because of this key isolation that does a
not sure if we can switch this conditionally to |
this is necessary for multi-parsers to not bail-out wrongly
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.
@TkDodo I added utilities to test bijectivity of custom multi-parsers. The overloads should take care of retro-compatibility, but if you see some issue, let me know.
To mirror the deprecation in SingleParser.
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, go for beta! 🚀
|
🎉 This PR is included in version 2.7.0-beta.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
MultiParsers allow key repetition in the URL.
E.g.: with the built-in
parseAsNativeArrayOf:Creating custom multi-parsers allow reducing the array of query values into more complex objects: