-
Notifications
You must be signed in to change notification settings - Fork 31.3k
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
lib: add validateThis and validateMissingArgs utilities #54405
base: main
Are you sure you want to change the base?
lib: add validateThis and validateMissingArgs utilities #54405
Conversation
b113369
to
ad77f8a
Compare
Added two new utility functions to `internal/validators`: - `validateThis`: Ensures that a context object (such as `this`) is correctly initialized and contains the expected private field. - `validateMissingArgs`: Validates that required arguments are provided, throwing an `ERR_MISSING_ARGS` error if any are undefined.
ad77f8a
to
dae16a6
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #54405 +/- ##
==========================================
+ Coverage 87.09% 88.41% +1.31%
==========================================
Files 648 652 +4
Lines 182216 186633 +4417
Branches 34969 36063 +1094
==========================================
+ Hits 158703 165012 +6309
+ Misses 16789 14897 -1892
Partials 6724 6724
|
lib/internal/validators.js
Outdated
const validateThis = hideStackFrames((context, className, privateField) => { | ||
if (typeof context !== 'object' || context === null || !(privateField in context)) { | ||
throw new ERR_INVALID_THIS(className); | ||
} | ||
}); |
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 suggest focusing on covering only
validateThis
in this PR and addressing the other utility in a separate PR. - It would be helpful for reviewers to include the code parts where
validateThis
is used (e.g.,lib/internal/url.js
). validateThis
andvalidateInternalField
perform similar functions, but throw different errors under same conditions. It might be worth considering unifying them to throw the same error in the future.
node/lib/internal/validators.js
Lines 529 to 533 in 75741a1
const validateInternalField = hideStackFrames((object, fieldKey, className) => { | |
if (typeof object !== 'object' || object === null || !ObjectPrototypeHasOwnProperty(object, fieldKey)) { | |
throw new ERR_INVALID_ARG_TYPE('this', className, object); | |
} | |
}); |
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.
As your suggestion, I am focusing on covering only validateThis in this PR :)
The function replaces repetitive this validation code in various internal modules, notably in internal/url.js, enhancing code consistency and maintainability. Before: get(name) {
if (typeof this !== 'object' || this === null || !(#searchParams in this))
throw new ERR_INVALID_THIS('URLSearchParams');
// ...
}
next() {
if (typeof this !== 'object' || this === null || !(#target in this))
throw new ERR_INVALID_THIS('URLSearchParamsIterator');
// ...
} After: get(name) {
validateThis(this, 'URLSearchParams');
// ...
}
next() {
validateThis(this, 'URLSearchParamsIterator');
// ...
} |
It could be apply followed Methods in URLSearchParams class: append, delete, get, getAll, next etc.. |
Added two new utility functions to
internal/validators