errors: add ERR_INVALID_OPT_TYPE and support options in validators.js #31251

lundibundi · 2020-01-07T20:56:10Z

Description in the commit messages.

Also, I considered adding helper functions like validateBufferOpt to avoid having to write an object with type: 'option' every time, WDYT?

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added (do we have a doc for validators.js?)
commit message follows commit guidelines

/cc @BridgeAR

lundibundi · 2020-01-07T20:57:42Z

lib/internal/errors.js

+  if (valueType !== undefined) {
+    msg += `"${name}" ${valueType} `;


The only thing added to this function (along with the fourth argument), otherwise a copy of the ERR_INVALID_ARG_TYPE function.

nodejs-github-bot · 2020-01-07T20:59:19Z

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

BridgeAR · 2020-01-07T21:07:12Z

I actually forgot that ERR_INVALID_ARG_TYPE already has some kind of support for properties: By passing through options.foobar the error message prints property instead of argument.

BridgeAR · 2020-01-07T21:11:14Z

It does of course not change the error code, so it's still a bit different. It would have probably been ideal to name the error ERR_INVALID_TYPE and then just add the information about being an argument or property in the message. But that's quite a breaking change...

I have to think about adding a completely new error code as implemented here. So far I have not made up my mind if I think it's good or bad.

lundibundi · 2020-01-07T21:55:37Z

It does of course not change the error code, so it's still a bit different. It would have probably been ideal to name the error ERR_INVALID_TYPE and then just add the information about being an argument or property in the message. But that's quite a breaking change...

That's what I was thinking initially but considering the amount of ERR_INVALID_ARG_TYPE errors we already have decided that it was not worth it, the breakage is quite large for a 'generalization' change. Though I'd actually prefer this approach if we can make it.

tniessen · 2020-01-18T15:09:26Z

Context: #31178 (comment)

This adds `ERR_INVALID_OPT_TYPE` error that is the equivalent of `ERR_INVALID_ARG_TYPE` for options.

`name` argument is now able to be and object with a `name` and `type` keys. If `type` is `'option'` then `ERR_INVALID_OPT_TYPE` and `ERR_INVALID_OPT_VALUE` errors will be used.

lundibundi · 2020-01-20T14:17:44Z

Rebased and fixed conflicts.

ping @BridgeAR @Trott @jasnell (is there a nodejs/ group for this kind of change?).

jasnell · 2020-01-21T14:30:06Z

(is there a nodejs/ group for this kind of change?

For error related PRs? Not really

jasnell · 2020-01-21T14:32:18Z

Overall I am fine with having a separate error code for options types but changing the code for existing errors makes this a semver-major.

lundibundi · 2020-01-21T15:38:21Z

@jasnell this doesn't change the existing error, the behavior should be exactly the same for ERR_INVALID_ARG_TYPE. I want this to be completely compatible so if you spotted a breaking change please tell me.

lundibundi · 2020-02-09T15:00:56Z

ping @BridgeAR @Trott @jasnell.

nodejs-github-bot · 2020-02-09T15:01:17Z

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

tniessen · 2020-02-12T14:36:45Z

So now we have ERR_INVALID_OPT_TYPE and ERR_INVALID_OPT_VALUE, but some values will throw other errors, e.g., ERR_OUT_OF_RANGE? This seems more and more confusing to me.

lundibundi · 2020-02-12T15:22:17Z

@tniessen I agree that it's not the best solution, but that's what we already have (with ERR_INVALID_ARG_TYPE etc), this PR just expands that to allow handling of option errors in the same way.

The current idea is that if we know a more specific error (e.g. out of range) then we use it, otherwise we just throw a generic invalid value or invalid type.
TBH I don't really see a point in specific out of range error in our current approach and IMO we could just always use invalid value and invalid type with specific error messages.

Trott · 2020-02-18T03:10:11Z

Pinging a few people based on git shortlog -n -s lib/internal/errors.js. @joyeecheung @addaleax @targos

lundibundi · 2020-05-31T12:54:22Z

ping @Trott @BridgeAR @jasnell @addaleax @targos @joyeecheung @tniessen
Should I rebase this to continue the discussion or did we hit a dead end as there doesn't seem to be enough attention to this one?

BridgeAR · 2020-05-31T13:24:36Z

I personally would keep it as is and I'm +-0 to this. What we do need is something to differentiate different error groups e.g., input validation errors, network errors, file system error, not supported error and similar.

jasnell · 2020-06-19T18:01:07Z

Not sure what the status is on this one but it needs a rebase

lundibundi · 2020-06-19T20:39:56Z

I can rebase this if we decide to move on with this, unfortunately, there is not enough feedback.
I'd prefer to have it to be able to streamline args\options handling and avoid discrepancies in errors.

lundibundi · 2020-08-07T20:21:44Z

Will try moving in the other direction with this.
#34070 (comment)

This will be a start to generalize all argument validation errors. As currently we throw ARG/OPT, OUT_OF_RANGE, and other more specific errors. The OPT errors didn't bring much to the errors as it's just another variant of ARG error which is sometimes more confusing (some of our code used OPT errors to denote just argument validation errors presumably because of similarity of OPT to 'option' and not 'options-object') and they don't specify the name of the options object where the invalid value is located. Much better approach would be to just specify path to the invalid value in the name of the value as it is done in this PR (i.e. 'options.format', 'options.publicKey.type' etc) Also since this decreases a variety of errors we have it'd be easier to reuse validation code across the codebase. Refs: nodejs#31251 Refs: nodejs#34070 (comment) Signed-off-by: Denys Otrishko <shishugi@gmail.com>

This will be a start to generalize all argument validation errors. As currently we throw ARG/OPT, OUT_OF_RANGE, and other more specific errors. The OPT errors didn't bring much to the errors as it's just another variant of ARG error which is sometimes more confusing (some of our code used OPT errors to denote just argument validation errors presumably because of similarity of OPT to 'option' and not 'options-object') and they don't specify the name of the options object where the invalid value is located. Much better approach would be to just specify path to the invalid value in the name of the value as it is done in this PR (i.e. 'options.format', 'options.publicKey.type' etc) Also since this decreases a variety of errors we have it'd be easier to reuse validation code across the codebase. Refs: #31251 Refs: #34070 (comment) Signed-off-by: Denys Otrishko <shishugi@gmail.com> PR-URL: #34682 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This will be a start to generalize all argument validation errors. As currently we throw ARG/OPT, OUT_OF_RANGE, and other more specific errors. The OPT errors didn't bring much to the errors as it's just another variant of ARG error which is sometimes more confusing (some of our code used OPT errors to denote just argument validation errors presumably because of similarity of OPT to 'option' and not 'options-object') and they don't specify the name of the options object where the invalid value is located. Much better approach would be to just specify path to the invalid value in the name of the value as it is done in this PR (i.e. 'options.format', 'options.publicKey.type' etc) Also since this decreases a variety of errors we have it'd be easier to reuse validation code across the codebase. Refs: nodejs#31251 Refs: nodejs#34070 (comment) Signed-off-by: Denys Otrishko <shishugi@gmail.com> PR-URL: nodejs#34682 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Jan 7, 2020

lundibundi commented Jan 7, 2020

View reviewed changes

lundibundi added 2 commits January 20, 2020 16:03

errors: introduce ERR_INVALID_OPT_TYPE for options of wrong type

5951669

This adds `ERR_INVALID_OPT_TYPE` error that is the equivalent of `ERR_INVALID_ARG_TYPE` for options.

errors: allow option validation with validators.js

bd91796

`name` argument is now able to be and object with a `name` and `type` keys. If `type` is `'option'` then `ERR_INVALID_OPT_TYPE` and `ERR_INVALID_OPT_VALUE` errors will be used.

lundibundi force-pushed the validators-support-options branch from 6cfb187 to bd91796 Compare January 20, 2020 14:15

jasnell approved these changes Jan 21, 2020

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 21, 2020

lundibundi removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 9, 2020

lundibundi added the review wanted PRs that need reviews. label Feb 9, 2020

lundibundi mentioned this pull request Feb 10, 2020

worker_threads: proper env and NODE_OPTIONS handling #31711

Closed

4 tasks

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19

lundibundi closed this Aug 7, 2020

lundibundi mentioned this pull request Aug 8, 2020

lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_ENCODING #34682

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

errors: add ERR_INVALID_OPT_TYPE and support options in validators.js #31251

errors: add ERR_INVALID_OPT_TYPE and support options in validators.js #31251

lundibundi commented Jan 7, 2020

lundibundi Jan 7, 2020

nodejs-github-bot commented Jan 7, 2020

BridgeAR commented Jan 7, 2020

BridgeAR commented Jan 7, 2020

lundibundi commented Jan 7, 2020

tniessen commented Jan 18, 2020

lundibundi commented Jan 20, 2020

jasnell commented Jan 21, 2020

jasnell commented Jan 21, 2020

lundibundi commented Jan 21, 2020 •

edited

lundibundi commented Feb 9, 2020

nodejs-github-bot commented Feb 9, 2020

tniessen commented Feb 12, 2020

lundibundi commented Feb 12, 2020

Trott commented Feb 18, 2020

lundibundi commented May 31, 2020

BridgeAR commented May 31, 2020

jasnell commented Jun 19, 2020

lundibundi commented Jun 19, 2020

lundibundi commented Aug 7, 2020

		if (valueType !== undefined) {
		msg += `"${name}" ${valueType} `;

errors: add ERR_INVALID_OPT_TYPE and support options in validators.js #31251

errors: add ERR_INVALID_OPT_TYPE and support options in validators.js #31251

Conversation

lundibundi commented Jan 7, 2020

Checklist

lundibundi Jan 7, 2020

Choose a reason for hiding this comment

nodejs-github-bot commented Jan 7, 2020

BridgeAR commented Jan 7, 2020

BridgeAR commented Jan 7, 2020

lundibundi commented Jan 7, 2020

tniessen commented Jan 18, 2020

lundibundi commented Jan 20, 2020

jasnell commented Jan 21, 2020

jasnell commented Jan 21, 2020

lundibundi commented Jan 21, 2020 • edited

lundibundi commented Feb 9, 2020

nodejs-github-bot commented Feb 9, 2020

tniessen commented Feb 12, 2020

lundibundi commented Feb 12, 2020

Trott commented Feb 18, 2020

lundibundi commented May 31, 2020

BridgeAR commented May 31, 2020

jasnell commented Jun 19, 2020

lundibundi commented Jun 19, 2020

lundibundi commented Aug 7, 2020

lundibundi commented Jan 21, 2020 •

edited