lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_ENCODING #34682

lundibundi · 2020-08-08T17:03:33Z

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

Since this is a semver-major anyway I'm also considering integrating ERR_OUT_OF_RANGE into ERR_INVALID_ARG_VALUE.RangeError as I don't see much benefit of it having a separate error code since everything will be stated in the message anyways, wdyt?

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

/cc @nodejs/crypto @nodejs/buffer @nodejs/http2 as most affected systems
/cc @jasnell @tniessen

nodejs-github-bot · 2020-08-08T17:03:37Z

Review requested:

@nodejs/http2
@nodejs/net

nodejs-github-bot · 2020-08-08T17:04:56Z

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

mcollina

lgtm

jasnell · 2020-08-10T14:30:55Z

@lundibundi :

...don't see much benefit of it having a separate error code since everything will be stated in the message anyways, wdyt?

That's where it gets a bit complicated. The reason we have error codes in the first place is to avoid users from having to rely on the message too much, which is what has led to changes in the error message having to be semver-major changes. I think the move to ERR_INVALID_ARG_VALUE.RangeError is fine in this particular case but I think we should supplement that also with additional error properties like error.detail.min and error.detail.max. Users can then use the combination of error code, type, and detail.

addaleax · 2020-08-10T15:30:02Z

but I think we should supplement that also with additional error properties like error.detail.min and error.detail.max. Users can then use the combination of error code, type, and detail.

Yeah, I think that’s where we messed up pretty badly when we introduced the error system – we made the errors parametrized, but only exposed those parameters via the message and not programmatically. That pushes people towards error message parsing, not away from it.

That being said, for this particular PR, I don’t have a strong opinion about whether we should change the error codes or not. It likely doesn’t matter because all of these are programming errors, not runtime errors (e.g. POSIX basically covers all of these through EINVAL – there’s no real point in further distinguishing, because whatever the program just did, it was not supposed to do that).

jasnell · 2020-08-10T15:41:20Z

@addaleax:

Yeah, I think that’s where we messed up pretty badly when we introduced the error system

Without question. At the time I started introducing the error codes, there was fairly strong resistance against doing too much here and I could never get momentum around adding the detail to the error objects. We definitely can and should tho!

BridgeAR · 2020-08-10T15:58:27Z

It is possible to add arbitrary properties to the errors, e.g.:

node/lib/internal/errors.js

Lines 1376 to 1381 in 64acae2

    
           E('ERR_TLS_CERT_ALTNAME_INVALID', function(reason, host, cert) { 
        
             this.reason = reason; 
        
             this.host = host; 
        
             this.cert = cert; 
        
             return `Hostname/IP does not match certificate's altnames: ${reason}`; 
        
           }, Error);

BridgeAR · 2020-08-10T16:00:08Z

I personally would keep ERR_OUT_OF_RANGE. What we do need is a way to group errors together to logical units such as NetworkErrors, InputError and similar. That would be most useful for users, since it's often not important if it's one exact error code or another as long as it's one of a specific group.

lundibundi · 2020-08-11T09:16:01Z

That's where it gets a bit complicated. The reason we have error codes in the first place is to avoid users from having to rely on the message too much, which is what has led to changes in the error message having to be semver-major changes. I think the move to ERR_INVALID_ARG_VALUE.RangeError is fine in this particular case but I think we should supplement that also with additional error properties like error.detail.min and error.detail.max. Users can then use the combination of error code, type, and detail.

While I agree that error-code separation is important and we definitely should rework the errors we have today to both include the values (like range, allowed values, erroring value etc) and the subsystem (or build new error hierarchy with subsystems though considering this is JS it may be hander to work with than just this.type === 'network' etc) as noted by @BridgeAR.

But for errors such as these (related to argument validation, which is basically EINVAL in POSIX), I don't see much use of having separate error codes since they are just programming errors and should not be programmatically\runtime handled (or am I missing some use-case?). This is exactly why I proposed removing ERR_OUT_OF_RANGE in favor of ERR_INVALID_ARG_VALUE.RangeError.

Without question. At the time I started introducing the error codes, there was fairly strong resistance against doing too much here and I could never get momentum around adding the detail to the error objects. We definitely can and should tho!

Yep, when I tried to make some changes to validation or errors in other PRs they didn't get much traction either, unfortunately. Should we start some sort of WG or at least add a group with people interested?

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>

lundibundi · 2020-08-18T09:20:53Z

Rebased and fixed conflicts, ready for review.
Also, instead of removing ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_ENCODING from docs I moved them to Legacy errors in errors.md.

The removal/replacement of ERR_OUT_OF_RANGE with custom properties on RangeError will be done in a separate PR after this lands to have a proper discussion there.

/cc @jasnell @Trott @addaleax @BridgeAR

Trott · 2020-08-23T11:47:26Z

@nodejs/tsc This is a semver-major so needs one more review from TSC.

Trott · 2020-08-23T11:49:21Z

CITGM (queued): https://ci.nodejs.org/job/citgm-smoker/2450/

doc/api/buffer.md

BridgeAR · 2020-08-23T16:53:32Z

lib/internal/errors.js

@@ -1080,7 +1080,8 @@ E('ERR_INVALID_ARG_VALUE', (name, value, reason = 'is invalid') => {
  if (inspected.length > 128) {
    inspected = `${inspected.slice(0, 128)}...`;
  }
-  return `The argument '${name}' ${reason}. Received ${inspected}`;
+  const type = name.includes('.') ? 'property' : 'argument';
+  return `The ${type} '${name}' ${reason}. Received ${inspected}`;


While changing this, we might also want to change the order to:

Suggested change

return `The ${type} '${name}' ${reason}. Received ${inspected}`;

return `The '${name}' ${type} ${reason}. Received ${inspected}`;

This seems nicer to read to me.

Looks better to me as well, let's ping a few more people since this incurs lots of test changes
@Trott @jasnell @addaleax @mcollina wdyt?

Edit: I can also do it in a follow-up PR to avoid delaying this PR further

…CODING Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot · 2020-08-24T15:36:09Z

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

mhdawson · 2020-09-04T21:36:12Z

@lundibundi thanks for the info.

github-actions · 2020-09-05T09:31:48Z

Commit Queue failed

- Loading data for nodejs/node/pull/34682
✔  Done loading data for nodejs/node/pull/34682
----------------------------------- PR info ------------------------------------
Title      lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_ENCODING (#34682)
Author     Denys Otrishko  (@lundibundi)
Branch     lundibundi:remove-opt-errors -> nodejs:master
Labels     author ready, lib / src, semver-major
Commits    2
 - lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_ENCODING
 - fixup! lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_EN…
Committers 2
 - Denys Otrishko 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/34682
Refs: https://github.com/nodejs/node/pull/31251
Reviewed-By: Matteo Collina 
Reviewed-By: Ruben Bridgewater 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/34682
Refs: https://github.com/nodejs/node/pull/31251
Reviewed-By: Matteo Collina 
Reviewed-By: Ruben Bridgewater 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2020-08-29T14:42:27Z: https://ci.nodejs.org/job/node-test-pull-request/32978/
   ℹ  Last CITGM CI on 2020-08-23T11:49:21Z: https://ci.nodejs.org/job/citgm-smoker/2450/
- Querying data for job/node-test-pull-request/32978/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Sat, 08 Aug 2020 17:03:33 GMT
   ✔  Approvals: 3
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/34682#pullrequestreview-463886678
   ✔  - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/34682#pullrequestreview-473024575
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/34682#pullrequestreview-473609826
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

…CODING

nodejs-github-bot · 2020-09-05T09:43:44Z

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

lundibundi · 2020-09-11T07:30:21Z

ping @Trott @jasnell @BridgeAR @mcollina this needs another review before it lands since there were changes to errors.md file to include version history in yaml (latest fixup!) as noted by @Trott.

Trott · 2020-09-11T14:27:44Z

ping @Trott @jasnell @BridgeAR @mcollina this needs another review before it lands since there were changes to errors.md file to include version history in yaml (latest fixup!) as noted by @Trott.

The fix-up commit looks good to me.

Trott

Fixup commit LGTM. Rubber-stamp on the rest of it based on other's who have already reviewed it.

github-actions · 2020-09-11T14:45:30Z

Commit Queue failed

- Loading data for nodejs/node/pull/34682
✔  Done loading data for nodejs/node/pull/34682
----------------------------------- PR info ------------------------------------
Title      lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_ENCODING (#34682)
Author     Denys Otrishko  (@lundibundi)
Branch     lundibundi:remove-opt-errors -> nodejs:master
Labels     author ready, lib / src, semver-major
Commits    3
 - lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_ENCODING
 - fixup! lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_EN…
 - fixup! lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_EN…
Committers 2
 - Denys Otrishko 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/34682
Refs: https://github.com/nodejs/node/pull/31251
Reviewed-By: Matteo Collina 
Reviewed-By: Ruben Bridgewater 
Reviewed-By: James M Snell 
Reviewed-By: Rich Trott 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/34682
Refs: https://github.com/nodejs/node/pull/31251
Reviewed-By: Matteo Collina 
Reviewed-By: Ruben Bridgewater 
Reviewed-By: James M Snell 
Reviewed-By: Rich Trott 
--------------------------------------------------------------------------------
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-09-05T09:43:44Z: https://ci.nodejs.org/job/node-test-pull-request/33065/
   ℹ  Last CITGM CI on 2020-09-05T09:31:48Z: https://ci.nodejs.org/job/citgm-smoker/2450/
- Querying data for job/node-test-pull-request/33065/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Sat, 08 Aug 2020 17:03:33 GMT
   ✔  Approvals: 4
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/34682#pullrequestreview-463886678
   ✔  - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/34682#pullrequestreview-473024575
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/34682#pullrequestreview-473609826
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/34682#pullrequestreview-486857257
--------------------------------------------------------------------------------
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/master up to date...
From https://github.com/nodejs/node
 * branch              master     -> FETCH_HEAD
✔  origin/master is now up-to-date
- Downloading patch for 34682
✔  Downloaded patch to /home/runner/work/node/node/.ncu/34682/patch
--------------------------------------------------------------------------------
error: patch failed: lib/internal/errors.js:1112
error: lib/internal/errors.js: patch does not apply
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Applying: lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_ENCODING
Patch failed at 0001 lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_ENCODING
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
--------------------------------------------------------------------------------
Applying: lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_ENCODING
error: sha1 information is lacking or useless (doc/api/buffer.md).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_ENCODING
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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>

lundibundi · 2020-09-11T15:00:16Z

Landed in c66e647

Refs: nodejs#34682

Notable changes: Deprecations and Removals: - **build**: remove --build-v8-with-gn configure option (Yang Guo) (#27576) - **build**: drop support for VS2017 (Michaël Zasso) (#33694) - **doc**: move DEP0018 to End-of-Life (Rich Trott) (#35316) - **fs**: deprecation warning on recursive rmdir (Ian Sutherland) (#35562) - **lib**: add EventTarget-related browser globals (Anna Henningsen) (#35496) - **net**: remove long deprecated server.connections property (James M Snell) (#33647) - **repl**: remove deprecated repl.memory function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.turnOffEditorMode() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.parseREPLKeyword() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated bufferedCommand property (Ruben Bridgewater) (#33286) - **repl**: remove deprecated .rli (Ruben Bridgewater) (#33286) - **src**: remove deprecated node debug command (James M Snell) (#33648) - **timers**: introduce timers/promises (James M Snell) (#33950) - **util**: change default value of `maxStringLength` to 10000 (unknown) (#32744) - **wasi**: drop --experimental-wasm-bigint requirement (Colin Ihrig) (#35415) npm 7 (#35631): Node.js 15 comes with a new major release of npm, npm 7. npm 7 comes with many new features - including npm workspaces and a new package-lock.json format. npm 7 also includes yarn.lock file support. One of the big changes in npm 7 is that peer dependencies are now installed by default. Throw On Unhandled Rejections (#33021): As of Node.js 15, the default mode for `unhandledRejection` is changed to `throw` (from `warn`). In `throw` mode, if an `unhandledRejection` hook is not set, the `unhandledRejection` is raised as an uncaught exception. Users that have an `unhandledRejection` hook should see no change in behavior, and it’s still possible to switch modes using the `--unhandled-rejections=mode` process flag. QUIC (#32379): Node.js 15 comes with experimental support QUIC, which can be enabled by compiling Node.js with the `--experimental-quic` configuration flag. The Node.js QUIC implementation is exposed by the core `net` module. V8 8.6 (#35415): The V8 JavaScript engine has been updated to V8 8.6 (V8 8.4 is the latest available in Node.js 14). Along with performance tweaks and improvements the V8 update also brings the following language features: * `Promise.any()` (from V8 8.5) * `AggregateError` (from V8 8.5) * `String.prototype.replaceAll()` (from V8 8.5) * Logical assignment operators `&&=`, `||=`, and `??=` (from V8 8.5) Other Notable Changes: - **assert**: add `assert/strict` alias module (ExE Boss) (#34001) - **dns**: add dns/promises alias (shisama) (#32953) - **fs**: reimplement read and write streams using stream.construct (Robert Nagy) (#29656) - **http2**: allow Host in HTTP/2 requests (Alba Mendez) (#34664) - **lib**: add EventTarget-related browser globals (Anna Henningsen) (#35496) - **lib**: unflag AbortController (James M Snell) (#33527) - **lib**: initial experimental AbortController implementation (James M Snell) (#33527) - **net**: autoDestroy Socket (Robert Nagy) (#31806) - **src**: disallow JS execution inside FreeEnvironment (Anna Henningsen) (#33874) - **stream**: construct (Robert Nagy) (#29656) - **worker**: make MessageEvent class more Web-compatible (Anna Henningsen) (#35496) Semver-Major Commits: - **assert**: add `assert/strict` alias module (ExE Boss) (#34001) - **build**: reset embedder string to "-node.0" (Michaël Zasso) (#35415) - **build**: remove --build-v8-with-gn configure option (Yang Guo) (#27576) - **build**: drop support for VS2017 (Michaël Zasso) (#33694) - **crypto**: refactoring internals, add WebCrypto (James M Snell) (#35093) - **crypto**: move node\_crypto files to src/crypto (James M Snell) (#35093) - **deps**: V8: cherry-pick d76abfed3512 (Michaël Zasso) (#35415) - **deps**: V8: cherry-pick 717543bbf0ef (Michaël Zasso) (#35415) - **deps**: V8: cherry-pick 6be2f6e26e8d (Michaël Zasso) (#35415) - **deps**: fix V8 build issue with inline methods (Jiawen Geng) (#35415) - **deps**: fix platform-embedded-file-writer-win for ARM64 (Michaël Zasso) (#35415) - **deps**: update V8 postmortem metadata script (Colin Ihrig) (#35415) - **deps**: update V8 to 8.6.395 (Michaël Zasso) (#35415) - **deps**: upgrade npm to 7.0.0 (Myles Borins) (#35631) - **deps**: update npm to 7.0.0-rc.3 (Myles Borins) (#35474) - **deps**: V8: cherry-pick 0d6debcc5f08 (Gus Caplan) (#33600) - **dns**: add dns/promises alias (shisama) (#32953) - **doc**: move DEP0018 to End-of-Life (Rich Trott) (#35316) - **doc**: update support macos version for 15.x (Ash Cripps) (#35022) - **fs**: deprecation warning on recursive rmdir (Ian Sutherland) (#35562) - **fs**: reimplement read and write streams using stream.construct (Robert Nagy) (#29656) - **http**: fixed socket.setEncoding fatal error (iskore) (#33405) - **http**: emit 'error' on aborted server request (Robert Nagy) (#33172) - **http**: cleanup end argument handling (Robert Nagy) (#31818) - **http2**: allow Host in HTTP/2 requests (Alba Mendez) (#34664) - **http2**: add `invalidheaders` test (Pranshu Srivastava) (#33161) - **http2**: refactor state code validation for the http2Stream class (rickyes) (#33535) - **http2**: header field valid checks (Pranshu Srivastava) (#33193) - **lib**: add EventTarget-related browser globals (Anna Henningsen) (#35496) - **lib**: remove ERR\_INVALID\_OPT\_VALUE and ERR\_INVALID\_OPT\_VALUE\_ENCODING (Denys Otrishko) (#34682) - **lib**: handle one of args case in ERR\_MISSING\_ARGS (Denys Otrishko) (#34022) - **lib**: remove NodeError from the prototype of errors with code (Michaël Zasso) (#33857) - **lib**: unflag AbortController (James M Snell) (#33527) - **lib**: initial experimental AbortController implementation (James M Snell) (#33527) - **net**: check args in net.connect() and socket.connect() calls (Denys Otrishko) (#34022) - **net**: remove long deprecated server.connections property (James M Snell) (#33647) - **net**: autoDestroy Socket (Robert Nagy) (#31806) - **process**: update v8 fast api calls usage (Maya Lekova) (#35415) - **process**: change default --unhandled-rejections=throw (Dan Fabulich) (#33021) - **process**: use v8 fast api calls for hrtime (Gus Caplan) (#33600) - **process**: delay throwing an error using `throwDeprecation` (Ruben Bridgewater) (#32312) - **repl**: remove deprecated repl.memory function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.turnOffEditorMode() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.parseREPLKeyword() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated bufferedCommand property (Ruben Bridgewater) (#33286) - **repl**: remove deprecated .rli (Ruben Bridgewater) (#33286) - **src**: implement NodePlatform::PostJob (Clemens Backes) (#35415) - **src**: update NODE\_MODULE\_VERSION to 88 (Michaël Zasso) (#35415) - **src**: error reporting on CPUUsage (Yash Ladha) (#34762) - **src**: use node:moduleName as builtin module filename (Michaël Zasso) (#35498) - **src**: enable wasm trap handler on windows (Gus Caplan) (#35033) - **src**: update NODE\_MODULE\_VERSION to 86 (Michaël Zasso) (#33579) - **src**: disallow JS execution inside FreeEnvironment (Anna Henningsen) (#33874) - **src**: remove \_third\_party\_main support (Anna Henningsen) (#33971) - **src**: remove deprecated node debug command (James M Snell) (#33648) - **src**: remove unused CancelPendingDelayedTasks (Anna Henningsen) (#32859) - **stream**: try to wait for flush to complete before 'finish' (Robert Nagy) (#34314) - **stream**: cleanup and fix Readable.wrap (Robert Nagy) (#34204) - **stream**: add promises version to utility functions (rickyes) (#33991) - **stream**: fix writable.end callback behavior (Robert Nagy) (#34101) - **stream**: construct (Robert Nagy) (#29656) - **stream**: write should throw on unknown encoding (Robert Nagy) (#33075) - **stream**: fix \_final and 'prefinish' timing (Robert Nagy) (#32780) - **stream**: simplify Transform stream implementation (Robert Nagy) (#32763) - **stream**: use callback to properly propagate error (Robert Nagy) (#29179) - **test**: update tests after increasing typed array size to 4GB (Kim-Anh Tran) (#35415) - **test**: fix tests for npm 7.0.0 (Myles Borins) (#35631) - **test**: fix test suite to work with npm 7 (Myles Borins) (#35474) - **test**: update WPT harness and tests (Michaël Zasso) (#33770) - **timers**: introduce timers/promises (James M Snell) (#33950) - **tools**: disable x86 safe exception handlers in V8 (Michaël Zasso) (#35415) - **tools**: update V8 gypfiles for 8.6 (Ujjwal Sharma) (#35415) - **tools**: update V8 gypfiles for 8.5 (Ujjwal Sharma) (#35415) - **url**: file URL path normalization (Daijiro Wachi) (#35477) - **url**: verify domain is not empty after "ToASCII" (Michaël Zasso) (#33770) - **url**: remove U+0000 case in the fragment state (Michaël Zasso) (#33770) - **url**: remove gopher from special schemes (Michaël Zasso) (#33325) - **url**: forbid lt and gt in url host code point (Yash Ladha) (#33328) - **util**: change default value of `maxStringLength` to 10000 (unknown) (#32744) - **wasi**: drop --experimental-wasm-bigint requirement (Colin Ihrig) (#35415) - **win, child_process**: sanitize env variables (Bartosz Sosnowski) (#35210) - **worker**: make MessageEvent class more Web-compatible (Anna Henningsen) (#35496) - **worker**: set trackUnmanagedFds to true by default (Anna Henningsen) (#34394) - **worker**: rename error code to be more accurate (Anna Henningsen) (#33872) PR-URL: #35014

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 lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 8, 2020

lundibundi added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 8, 2020

mcollina approved these changes Aug 9, 2020

View reviewed changes

Trott requested a review from addaleax August 10, 2020 15:25

lundibundi requested review from a team as code owners August 10, 2020 16:06

lundibundi requested a review from a team August 10, 2020 16:06

lundibundi force-pushed the remove-opt-errors branch from 2223943 to 6623cd1 Compare August 18, 2020 09:13

lundibundi force-pushed the remove-opt-errors branch from 6623cd1 to e595e60 Compare August 18, 2020 09:17

Trott requested a review from joyeecheung August 20, 2020 23:46

BridgeAR approved these changes Aug 23, 2020

View reviewed changes

fixup! lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_EN…

d5fb345

…CODING Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>

jasnell approved these changes Aug 24, 2020

View reviewed changes

lundibundi added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 24, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 24, 2020

This was referenced Aug 25, 2020

CI Reliability 2020-08-25 nodejs/reliability#54

Open

CI Reliability 2020-08-26 nodejs/reliability#55

Open

lundibundi added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 5, 2020

github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 5, 2020

fixup! lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_EN…

cdef566

…CODING

lundibundi removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2020

Trott approved these changes Sep 11, 2020

View reviewed changes

lundibundi added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 11, 2020

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 11, 2020

github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 11, 2020

lundibundi closed this Sep 11, 2020

lundibundi deleted the remove-opt-errors branch September 11, 2020 15:00

lundibundi removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 11, 2020

lundibundi added a commit to lundibundi/node that referenced this pull request Sep 11, 2020

lib: change ERR_INVALID_ARG_VALUE error message

3ec5b48

Refs: nodejs#34682

This was referenced Sep 11, 2020

lib: change ERR_INVALID_ARG_VALUE error message #35156

Closed

Tracking Issue: commit queue issues and feedback #34770

Open

BethGriggs mentioned this pull request Sep 14, 2020

2020-10-20, Version 15.0.0 (Current) #35014

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_ENCODING #34682

lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_ENCODING #34682

lundibundi commented Aug 8, 2020

nodejs-github-bot commented Aug 8, 2020

nodejs-github-bot commented Aug 8, 2020

mcollina left a comment

jasnell commented Aug 10, 2020

addaleax commented Aug 10, 2020

jasnell commented Aug 10, 2020

BridgeAR commented Aug 10, 2020

BridgeAR commented Aug 10, 2020

lundibundi commented Aug 11, 2020

lundibundi commented Aug 18, 2020

Trott commented Aug 23, 2020

Trott commented Aug 23, 2020

BridgeAR Aug 23, 2020

lundibundi Aug 24, 2020 •

edited

Loading

nodejs-github-bot commented Aug 24, 2020

mhdawson commented Sep 4, 2020

github-actions bot commented Sep 5, 2020

nodejs-github-bot commented Sep 5, 2020

lundibundi commented Sep 11, 2020

Trott commented Sep 11, 2020

Trott left a comment

github-actions bot commented Sep 11, 2020

lundibundi commented Sep 11, 2020

	return `The ${type} '${name}' ${reason}. Received ${inspected}`;
	return `The '${name}' ${type} ${reason}. Received ${inspected}`;

lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_ENCODING #34682

lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_ENCODING #34682

Conversation

lundibundi commented Aug 8, 2020

Checklist

nodejs-github-bot commented Aug 8, 2020

nodejs-github-bot commented Aug 8, 2020

mcollina left a comment

Choose a reason for hiding this comment

jasnell commented Aug 10, 2020

addaleax commented Aug 10, 2020

jasnell commented Aug 10, 2020

BridgeAR commented Aug 10, 2020

BridgeAR commented Aug 10, 2020

lundibundi commented Aug 11, 2020

lundibundi commented Aug 18, 2020

Trott commented Aug 23, 2020

Trott commented Aug 23, 2020

BridgeAR Aug 23, 2020

Choose a reason for hiding this comment

lundibundi Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

nodejs-github-bot commented Aug 24, 2020

mhdawson commented Sep 4, 2020

github-actions bot commented Sep 5, 2020

nodejs-github-bot commented Sep 5, 2020

lundibundi commented Sep 11, 2020

Trott commented Sep 11, 2020

Trott left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 11, 2020

lundibundi commented Sep 11, 2020

lundibundi Aug 24, 2020 •

edited

Loading