lib: refactor to use validators in http2

[debadree25]

Attempted to refactor usage of validators similar to the referenced PR

Refs: #46101

[nodejs-github-bot]

Review requested:

• @nodejs/http2
• @nodejs/net

[VoltrexKeyva]

The proposed changes in the http2/core.js file changes not only the validation behavior [1] but also changes the error code from ERR_INVALID_ARG_VALUE to ERR_INVALID_ARG_TYPE which can be considered semver-major if users depend on error codes to handle errors.

1: the current behavior skips validation if the value is undefined, but the proposed change does not, which can affect performance and also validate values unexpectedly.

[debadree25]

1: the current behavior skips validation if the value is undefined, but the proposed change does not, which can affect performance and also validate values unexpectedly.

Didn't get you, the checks for undefined are still there, could you point to the line where you see this issue?

[debadree25]

the error code from ERR_INVALID_ARG_VALUE to ERR_INVALID_ARG_TYPE which can be considered semver-major if users depend on error codes to handle errors.

Yes I think we need to add the semver-major label on the PR

[VoltrexKeyva]

Didn't get you, the checks for undefined are still there, could you point to the line where you see this issue?

I meant that the values (such as the value of options.weight) are not validated in this case if they're undefined, such as in the lines you just changed:

if (options.weight === undefined) {
  options.weight = NGHTTP2_DEFAULT_WEIGHT;
} else if (typeof options.weight !== 'number') {
  throw new ERR_INVALID_ARG_VALUE('options.weight', options.weight);
}

The check in the else if clause isn't reached and is ignored if the conditional in the first if statement passes, but with your proposed changes, it'll validate the value regardless.

---

Basically, let's say that options.weight is undefined, here's what happens in the current code:

if (options.weight === undefined) { // The value is `undefined` as we considered, so this part will execute.
  options.weight = NGHTTP2_DEFAULT_WEIGHT;
} else if (typeof options.weight !== 'number') { // This part will NOT be reached, because the previous condition passed, and so this is ignored.
  throw new ERR_INVALID_ARG_VALUE('options.weight', options.weight);
}

But with your proposed change:

if (options.weight === undefined) {
  options.weight = NGHTTP2_DEFAULT_WEIGHT;
}

validateNumber(options.weight, 'options.weight');

options.weight will still be validated even if it's undefined, unlike the current behavior, which can affect the performance. Even though it's value is reassigned, it's reassigned to the expected type, so validation here would be redundant.

[debadree25]

Understood, fixing that

[debadree25]

Updated, is it ok now @VoltrexKeyva

[VoltrexKeyva]

Applying semver-major PRs that contain breaking changes and should be released in the next major version. label for the error code change as mentioned.

[ShogunPanda]

LGTM!

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/49063/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/49100/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/49105/

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/46174
✔  Done loading data for nodejs/node/pull/46174
----------------------------------- PR info ------------------------------------
Title      lib: refactor to use validators in http2 (#46174)
Author     Debadree Chatterjee  (@debadree25)
Branch     debadree25:ft/refactor-to-validate-boolean -> nodejs:main
Labels     semver-major, http2, needs-ci, commit-queue-squash
Commits    4
 - lib: refactor to use validators in http2
 - fixup! lint fix
 - fixup! prevent unnecessary checking
 - fixup! remove message checking in test!
Committers 1
 - Debadree Chatterjee 
PR-URL: https://github.com/nodejs/node/pull/46174
Refs: https://github.com/nodejs/node/pull/46101
Reviewed-By: Paolo Insogna 
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/46174
Refs: https://github.com/nodejs/node/pull/46101
Reviewed-By: Paolo Insogna 
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 11 Jan 2023 18:30:01 GMT
   ✔  Approvals: 3
   ✔  - Paolo Insogna (@ShogunPanda): https://github.com/nodejs/node/pull/46174#pullrequestreview-1245228218
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/46174#pullrequestreview-1245384252
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/46174#pullrequestreview-1250639453
   ✖  semver-major requires at least 2 TSC approvals
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-01-20T14:05:16Z: https://ci.nodejs.org/job/node-test-pull-request/49105/
- Querying data for job/node-test-pull-request/49105/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/3976348462

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/46174
✔  Done loading data for nodejs/node/pull/46174
----------------------------------- PR info ------------------------------------
Title      lib: refactor to use validators in http2 (#46174)
Author     Debadree Chatterjee  (@debadree25)
Branch     debadree25:ft/refactor-to-validate-boolean -> nodejs:main
Labels     semver-major, http2, needs-ci, commit-queue-squash
Commits    4
 - lib: refactor to use validators in http2
 - fixup! lint fix
 - fixup! prevent unnecessary checking
 - fixup! remove message checking in test!
Committers 1
 - Debadree Chatterjee 
PR-URL: https://github.com/nodejs/node/pull/46174
Refs: https://github.com/nodejs/node/pull/46101
Reviewed-By: Paolo Insogna 
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
Reviewed-By: Minwoo Jung 
Reviewed-By: Trivikram Kamat 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/46174
Refs: https://github.com/nodejs/node/pull/46101
Reviewed-By: Paolo Insogna 
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
Reviewed-By: Minwoo Jung 
Reviewed-By: Trivikram Kamat 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 11 Jan 2023 18:30:01 GMT
   ✔  Approvals: 5
   ✔  - Paolo Insogna (@ShogunPanda): https://github.com/nodejs/node/pull/46174#pullrequestreview-1245228218
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/46174#pullrequestreview-1245384252
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/46174#pullrequestreview-1250639453
   ✔  - Minwoo Jung (@JungMinu): https://github.com/nodejs/node/pull/46174#pullrequestreview-1264737330
   ✔  - Trivikram Kamat (@trivikr): https://github.com/nodejs/node/pull/46174#pullrequestreview-1266710305
   ✖  semver-major requires at least 2 TSC approvals
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-01-21T19:57:12Z: https://ci.nodejs.org/job/node-test-pull-request/49105/
- Querying data for job/node-test-pull-request/49105/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/3994541670

[mcollina]

@nodejs/tsc this requires another TSC review because it's semver-major

[nodejs-github-bot]

Landed in e4d641f