-
-
Notifications
You must be signed in to change notification settings - Fork 100
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
fix: Log an error in development if URI size of 2000 characters is exceeded. #747
Conversation
Someone is attempting to deploy a commit to the 47ng Team on Vercel. A member of the Team first needs to authorize it. |
@franky47 wasn't to sure about the placement of the log, let me know. |
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.
Thanks!
One global thing to change in addition to individual suggestions is the term URL rather than URI, to be consistent with the rest of the codebase (URLSearchParams, window.location etc).
packages/nuqs/src/update-queue.ts
Outdated
@@ -153,6 +153,9 @@ function flushUpdateQueue( | |||
scroll: options.scroll, | |||
shallow: options.shallow | |||
}) | |||
if (URIIsTooLong() && process.env.NODE_ENV === 'development') { |
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.
question: Does process.env.NODE_ENV
work everywhere? Eg: in Vite, other bundlers, or in non-Node.js runtimes (Deno / Bun).
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 know that Bun is designed with Node.js in mind so they expose the node env over process.env just like node does. AFAIK deno doesn't support this.
It works for vite and most other bundlers use plugins to expose process.env aswell.
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.
We can remove the second check but then you would have those warnings in prod builds.
packages/nuqs/src/utils.ts
Outdated
export function URIIsTooLong() { | ||
return window.location.href.length > URI_MAX_LENGTH | ||
} |
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.
Reading from window.location
is not guaranteed to have the correct search params values, the updateUrl
function called beforehand may call async APIs where the URL will be updated later.
suggestion: Optimistically apply the search params to the current URL and check on that (this function should probably live in url-encoding.ts
):
export function URIIsTooLong() { | |
return window.location.href.length > URI_MAX_LENGTH | |
} | |
export function URIIsTooLong(search: URLSearchParams) { | |
const url = new URL(window.location) | |
url.search = renderQueryString(search) | |
return url.href.length > URI_MAX_LENGTH | |
} |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@franky47 I refactored according to your remarks, please have a look if this works for you. |
Co-authored-by: François Best <[email protected]>
Co-authored-by: François Best <[email protected]>
Co-authored-by: François Best <[email protected]>
🎉 This PR is included in version 2.1.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
As discussed here.
Error logging for when a user stores more than 2000 characters of state in a URI.
nuqs-e2e-test-bench.mp4