Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rayark1
Copy link
Contributor

@rayark1 rayark1 commented Aug 16, 2024

Added two new utility functions to internal/validators

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Aug 16, 2024
@rayark1 rayark1 force-pushed the validators/add-validateThis-validateMissingArgs branch 2 times, most recently from b113369 to ad77f8a Compare August 16, 2024 09:02
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.
@rayark1 rayark1 force-pushed the validators/add-validateThis-validateMissingArgs branch from ad77f8a to dae16a6 Compare August 16, 2024 09:03
@rayark1 rayark1 changed the title validators: add validateThis and validateMissingArgs utilities lib: add validateThis and validateMissingArgs utilities Aug 16, 2024
Copy link

codecov bot commented Aug 16, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 7 lines in your changes missing coverage. Please review.

Project coverage is 88.41%. Comparing base (02b3095) to head (6afc36d).
Report is 564 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/validators.js 66.66% 7 Missing ⚠️
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              
Files with missing lines Coverage Δ
lib/internal/validators.js 97.15% <66.66%> (-2.52%) ⬇️

... and 303 files with indirect coverage changes

@avivkeller avivkeller added util Issues and PRs related to the built-in util module. and removed util Issues and PRs related to the built-in util module. labels Aug 16, 2024
Comment on lines 512 to 516
const validateThis = hideStackFrames((context, className, privateField) => {
if (typeof context !== 'object' || context === null || !(privateField in context)) {
throw new ERR_INVALID_THIS(className);
}
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I suggest focusing on covering only validateThis in this PR and addressing the other utility in a separate PR.
  2. It would be helpful for reviewers to include the code parts where validateThis is used (e.g.,lib/internal/url.js).
  3. validateThis and validateInternalField 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.

const validateInternalField = hideStackFrames((object, fieldKey, className) => {
if (typeof object !== 'object' || object === null || !ObjectPrototypeHasOwnProperty(object, fieldKey)) {
throw new ERR_INVALID_ARG_TYPE('this', className, object);
}
});

Copy link
Contributor Author

@rayark1 rayark1 Oct 6, 2024

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 :)

@rayark1
Copy link
Contributor Author

rayark1 commented Oct 6, 2024

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');
  // ...
}

@rayark1
Copy link
Contributor Author

rayark1 commented Oct 6, 2024

It could be apply followed Methods in URLSearchParams class: append, delete, get, getAll, next etc..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants