fix: don't fail parse if called again with identical input #557
Conversation
|
@rikbrown is attempting to deploy a commit to the 47ng Team on Vercel. A member of the Team first needs to authorize it. |
rikbrown
force-pushed
the
next
branch
2 times, most recently
from
593147e
to
5c79fa3
Compare
|
Actually I just realised you have e2e tests setup, let me update them too. |
added a simple one |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Thank you!
It looks good overall, apart from the storage of the input reference object (see comment).
I can't figure out how to approve CI to run on 3rd party PRs (the button was there this morning but gone now...), I'll see what I can do. Edit: note to self: this is to be done in the Actions, not on the PR itself. Re-edit: nope, still only applies to the first commit, not to subsequent (force-)pushes..
packages/nuqs/src/cache.ts
Outdated
| @@ -21,16 +21,33 @@ export function createSearchParamsCache< | |||
| // whereas a simple object would be bound to the lifecycle of the process, | |||
| // which may be reused between requests in a serverless environment | |||
| // (warm lambdas on Vercel or AWS). | |||
| const getCache = cache<() => Partial<ParsedSearchParams>>(() => ({})) | |||
| const getCache = cache< | |||
| () => Partial<ParsedSearchParams> & { _orig?: SearchParams } | |||
There was a problem hiding this comment.
suggestion: Rather than storing the input in the same storage space as the rest of search params (which can be spoofed if someone specifies /?_orig=whatever), we could use a non-exported Symbol which would never match anything provided in the URL.
Best would be to completely separate this from storage, for example by setting it as a Symbol-accessed property of the returned search params cache interface (so it can be accessed from the outside).
suggestion: I would also choose a more explicit name, like parseInput. If _orig stands for origin (or original?), this has a different meaning for code that deals with URLs.
There was a problem hiding this comment.
Thanks for those suggestions, makes a lot of sense. _orig was meant to stand for original but yes great point on it conflicting w/ the real search params namespace.
Symbols are a bit new to me (but make a lot of sense for this!) - I think I've done what you suggested and it seems to work. I separated the cache out into a new type with the parsed search params + the input (using a non-exported symbol for the key). LMK if this is right or I misunderstood. (Also I used $input for the symbol name as I saw in some examples the $ prefix being used to designate symbols but if you prefer a plain variable name I can change it).
Thanks for the fast review!
packages/nuqs/src/cache.ts
Outdated
| return Object.freeze(c) as Readonly<ParsedSearchParams> | ||
|
|
||
| c[$input] = searchParams | ||
| Object.freeze(c) |
There was a problem hiding this comment.
Looks great! The only detail now is that Object.freeze only does so one level deep, so this should be called on c.searchParams (and also updated in the check above), as we want to freeze the storage values (freezing the Cache object itself doesn't do much as nobody has access to it from the outside).
|
🎉 This PR is included in version 1.17.3-beta.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.17.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
It should be possible to use nuqs to parse search params in multiple places in the same request (e.g. page +
generateMetadata) - as long as the input is identical (which it should be).This change stores the original searchparams object passed to parse alongside the parsed data. If
parseis called again we check for referential equality and if identical we just return the already cached data.note: that this will still fail if a different object with the same contents is passed. fortunately next.js uses the same object for search params in the same request so it solves the original use case. I didn't want to get more clever and do deeper comparison for simplicity/perf.
i stored the original object as a new field in the existing cache object (not exposed to users).
see #556
Testing
generateMetadatato the cache e2e test performing parse again (the fact it doesn't fail felt sufficient here, but I did also check the value of one of the params by setting the title).