timers: Migrate to use internal/errors #14659

starkwang · 2017-08-07T07:08:57Z

ref: #11273

Migrate the timers module to use internal/errors. And some tests are added for timers.enroll().

Checklist

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

Affected core subsystem(s)

timers, test, errors

tniessen · 2017-08-07T09:41:15Z

lib/timers.js

  }

  if (msecs < 0 || !isFinite(msecs)) {
-    throw new RangeError('"msecs" argument must be ' +
-                         'a non-negative finite number');
+    throw new errors.RangeError('ERR_INVALID_OPT_VALUE', 'msecs', msecs);


This does not contain information about the allowed range anymore.

The motivation is to try to have minimal changes in the new error message
Suggestion (if it works):

errors.TypeError('ERR_INVALID_ARG_TYPE', 'msecs', 'a non-negative finite number', msecs);

The error message will become like:

RangeError [ERR_INVALID_ARG_TYPE]: The "msecs" argument must be of type a non-negative finite number

IMHO, we should add a new error code (e.g. ERR_OPT_VALUE_OUT_OF_RANGE), which can be reused in other modules. (e.g. https://github.com/nodejs/node/blob/master/lib/buffer.js#L244)

Sounds good 👍 (New error code)

tniessen · 2017-08-07T09:41:43Z

test/parallel/test-timers-enroll-invalid-msecs.js

+  [],
+  'foo',
+  () => { },
+  Symbol('foo'),


Style nit: Trailing comma.

Didn't mean to approve, sorry.

refack

💯

refack · 2017-08-07T15:57:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9519/
@jasnell @fhinkel @Trott @mhdawson @mcollina PTAL

tniessen · 2017-08-07T17:35:31Z

lib/internal/errors.js

@@ -229,6 +229,8 @@ E('ERR_NAPI_CONS_PROTOTYPE_OBJECT', 'Constructor.prototype must be an object');
 E('ERR_NO_CRYPTO', 'Node.js is not compiled with OpenSSL crypto support');
 E('ERR_NO_ICU', '%s is not supported on Node.js compiled without ICU');
 E('ERR_NO_LONGER_SUPPORTED', '%s is no longer supported');
+E('ERR_OPT_VALUE_OUT_OF_RANGE', (name, expected ,actual) =>
+  `The value of "${name}" must be ${expected}. Recieved "${actual}"`);


Spelling: Received

tniessen · 2017-08-07T17:37:27Z

test/parallel/test-internal-errors.js

@@ -257,6 +257,11 @@ assert.strictEqual(
 );

 assert.strictEqual(
+  errors.message('ERR_OPT_VALUE_OUT_OF_RANGE', ['A', 'some values', 'B']),
+  'The value of "A" must be some values. Recieved "B"'


refack · 2017-08-07T17:42:27Z

lib/internal/errors.js

@@ -229,6 +229,8 @@ E('ERR_NAPI_CONS_PROTOTYPE_OBJECT', 'Constructor.prototype must be an object');
 E('ERR_NO_CRYPTO', 'Node.js is not compiled with OpenSSL crypto support');
 E('ERR_NO_ICU', '%s is not supported on Node.js compiled without ICU');
 E('ERR_NO_LONGER_SUPPORTED', '%s is no longer supported');
+E('ERR_OPT_VALUE_OUT_OF_RANGE', (name, expected ,actual) =>


lint in L232:

not ok 3 - /usr/home/iojs/build/workspace/node-test-linter/lib/internal/errors.js --- message: 'There should be no space before '',''.' severity: error data: line: 232 column: 49 ruleId: comma-spacing messages: - message: 'A space is required after '',''.' severity: error data: line: 232 column: 49 ruleId: comma-spacing ...

starkwang · 2017-08-08T00:09:10Z

Fixed typo and lint

BridgeAR

A small nit, LGTM otherwise

BridgeAR · 2017-08-08T04:54:50Z

lib/internal/errors.js

@@ -229,6 +229,8 @@ E('ERR_NAPI_CONS_PROTOTYPE_OBJECT', 'Constructor.prototype must be an object');
 E('ERR_NO_CRYPTO', 'Node.js is not compiled with OpenSSL crypto support');
 E('ERR_NO_ICU', '%s is not supported on Node.js compiled without ICU');
 E('ERR_NO_LONGER_SUPPORTED', '%s is no longer supported');
+E('ERR_OPT_VALUE_OUT_OF_RANGE', (name, expected, actual) =>
+  `The value of "${name}" must be ${expected}. Received "${actual}"`);


To keep consistency it would be best to use %s and rely on util.format instead of using a function.

Fishrock123

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

req.socket._hadError should be set before emitting the error event. PR-URL: #14659 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

tniessen · 2017-08-16T15:13:23Z

The above commit 620ba41 should not have referenced this PR, sorry for any confusion.
By the way, we still need someone from @nodejs/ctc to sign this off.

thefourtheye · 2017-08-16T16:55:57Z

doc/api/errors.md

@@ -1023,6 +1023,11 @@ Used when a Node.js API is called in an unsupported manner.

 For example: `Buffer.write(string, encoding, offset[, length])`

+<a id="ERR_OPT_VALUE_OUT_OF_RANGE"></a>
+### ERR_OPT_VALUE_OUT_OF_RANGE


Nit: OPT_VALUE is not intuitive. ERR_VALUE_OUT_OF_RANGE would be better I guess.

thefourtheye · 2017-08-16T16:57:10Z

test/parallel/test-timers-enroll-invalid-msecs.js

+
+[
+  -1,
+  Infinity


Can we include zero, for boundary checking and NaN, for special case?

Zero is valid for argument msecs. I've added NaN for special case.

req.socket._hadError should be set before emitting the error event. PR-URL: nodejs/node#14659 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

ref: nodejs#11273

BridgeAR

Still LGTM

BridgeAR · 2017-08-27T14:19:11Z

Landed in 4d893e0

PR-URL: #14659 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs/node#14659 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

* **async_hooks** * Older experimental `async_hooks` APIs have been removed [[`d731369b1d`](d731369b1d)] **(SEMVER-MAJOR)** [#14414](#14414) * **Errors** * Multiple built in modules have been migrated to use static error codes * **Domains** * The long deprecated `.dispose()` method has been removed [[`602fd36d95`](602fd36d95)] **(SEMVER-MAJOR)** [#15412](#15412) * **File system** * `fs.ReadStream` and `fs.WriteStream` now make use of `.destroy()` [[`e5c290bed9`](e5c290bed9)] **(SEMVER-MAJOR)** [#15407](#15407) * `fs` callbacks are now invoked with an undefined `this` context [[`2249234fee`](2249234fee)] **(SEMVER-MAJOR)** [#14645](#14645) * **HTTP** * Socket timeout is set when the socket connects [[`10be20a0e8`](10be20a0e8)] **(SEMVER-MAJOR)** [#8895](#8895) * A bug causing the request `error` event to fire twice has been fixed [[`620ba41694`](620ba41694)] **(SEMVER-MAJOR)** [#14659](#14659) * The `pipe` method on `OutgoingMessage` has been disabled [[`156549d8ff`](156549d8ff)] **(SEMVER-MAJOR)** [#14358](#14358) * **HTTP/2** * The `--expose-http2` command-line argument is no longer required [[`f55ee6e24a`](f55ee6e24a)] **(SEMVER-MAJOR)** [#15535](#15535) * **Internationalization** * The `Intl.v8BreakIterator` class has been removed [[`668ad44922`](668ad44922)] **(SEMVER-MAJOR)** [#15238](#15238) * **OS** * `os.EOL` is now read-only [[`f6caeb9526`](f6caeb9526)] **(SEMVER-MAJOR)** [#14622](#14622) * **Process** * It is now possible to pass additional flags to `dlopen` [[`5f22375922`](5f22375922)] **(SEMVER-MAJOR)** [#12794](#12794) * **Timers** * Using a timeout duration larger than 32-bits will now emit a warning [[`ce3586da31`](ce3586da31)] **(SEMVER-MAJOR)** [#15627](#15627) * **TLS** * `parseCertString` has been deprecated [[`468110b327`](468110b327)] **(SEMVER-MAJOR)** [#14249](#14249) * Type-checking for `key`, `cert`, and `ca` options has been added [[`a7dccd040d`](a7dccd040d)] **(SEMVER-MAJOR)** [#14807](#14807)

nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Aug 7, 2017

tniessen previously approved these changes Aug 7, 2017

View reviewed changes

refack added errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. labels Aug 7, 2017

refack self-assigned this Aug 7, 2017

starkwang force-pushed the timers-internal-errors branch 3 times, most recently from 46172ab to 65cd92a Compare August 7, 2017 15:33

refack approved these changes Aug 7, 2017

View reviewed changes

tniessen reviewed Aug 7, 2017

View reviewed changes

refack reviewed Aug 7, 2017

View reviewed changes

starkwang force-pushed the timers-internal-errors branch 2 times, most recently from 7727304 to cd2e3ef Compare August 8, 2017 00:08

jasnell mentioned this pull request Aug 8, 2017

Tracking Issue: Migrate errors to internal/errors.js #11273

Closed

80 tasks

BridgeAR reviewed Aug 8, 2017

View reviewed changes

starkwang force-pushed the timers-internal-errors branch from cd2e3ef to fc7e71d Compare August 8, 2017 06:14

Fishrock123 approved these changes Aug 14, 2017

View reviewed changes

jasnell approved these changes Aug 16, 2017

View reviewed changes

thefourtheye approved these changes Aug 16, 2017

View reviewed changes

starkwang force-pushed the timers-internal-errors branch from fc7e71d to b042d19 Compare August 18, 2017 02:04

timers: Migrate to use internal/errors

7a09080

ref: nodejs#11273

starkwang force-pushed the timers-internal-errors branch from b042d19 to 7a09080 Compare August 24, 2017 05:21

BridgeAR approved these changes Aug 27, 2017

View reviewed changes

BridgeAR closed this Aug 27, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

refack removed their assignment Oct 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

timers: Migrate to use internal/errors #14659

timers: Migrate to use internal/errors #14659

starkwang commented Aug 7, 2017 •

edited

Loading

tniessen Aug 7, 2017

refack Aug 7, 2017

starkwang Aug 7, 2017

refack Aug 7, 2017 •

edited

Loading

tniessen Aug 7, 2017

refack left a comment

refack commented Aug 7, 2017

tniessen Aug 7, 2017

tniessen Aug 7, 2017

refack Aug 7, 2017

starkwang commented Aug 8, 2017

BridgeAR left a comment

BridgeAR Aug 8, 2017

Fishrock123 left a comment

tniessen commented Aug 16, 2017

thefourtheye Aug 16, 2017

thefourtheye Aug 16, 2017

starkwang Aug 18, 2017

BridgeAR left a comment

BridgeAR commented Aug 27, 2017

timers: Migrate to use internal/errors #14659

timers: Migrate to use internal/errors #14659

Conversation

starkwang commented Aug 7, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

refack commented Aug 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starkwang commented Aug 8, 2017

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 left a comment

Choose a reason for hiding this comment

tniessen commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR commented Aug 27, 2017

starkwang commented Aug 7, 2017 •

edited

Loading

refack Aug 7, 2017 •

edited

Loading