-
Notifications
You must be signed in to change notification settings - Fork 27k
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(ts): ReadonlyURLSearchParams
should extend URLSearchParams
#61419
Conversation
Tests Passed |
Stats from current PRDefault BuildGeneral
Client Bundles (main, webpack)
Legacy Client Bundles (polyfills)
Client Pages
Client Build Manifests
Rendered Page Sizes
Edge SSR bundle Size
Middleware size
Next Runtimes
Diff detailsDiff for page.jsDiff too large to display Diff for 68-HASH.jsDiff too large to display Diff for app-page-exp..ntime.dev.jsDiff too large to display Diff for app-page.runtime.dev.jsDiff too large to display |
[Symbol.iterator]() { | ||
return this[INTERNAL_URLSEARCHPARAMS_INSTANCE][Symbol.iterator]() | ||
/** @internal */ | ||
class ReadonlyURLSearchParamsError extends Error { |
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.
This results in more code output right? The internal methods can't be minified whereas the function version can be minified
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 believe this should be an acceptable size change.
Generally, we should start using proper Errors everywhere to improve the codebase readability/type safety, etc.
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 with @balazsorban44 here in that improving the hygiene around error management, creation, is a huge area within the codebase that we could use some more rigid structure around. While we might have some minification losses, cases where we're creating error classes are few and far between.
What?
Let the developer check the instance of
ReadonlyURLSearchParams
to match againstURLSearchParams
Why?
useSearchParams()
's return type isReadonlyURLSearchParams
which implements all the methods ofURLSearchParams
, therefore its type should be extended fromURLSearchParams
as well. Deprecated methods are also implemented to throw an error, so no runtime behavior is being changedHow?
Mark the unavailable methods as
@deprecated
which will visually mark them in IDEs:This is similar how
ReadonlyHeaders
extendsHeaders
, added in: #49075Slack thread
Closes NEXT-2305