dgram: convert to using internal/errors#12926

mhdawson · 2017-05-09T18:32:44Z

Convert lib/dgram.js over to using lib/internal/errors.js
for generating Errors. See using-internal-errors.md for more details.

I have not addressed the cases that use errnoException() and
exceptionWithHostPort() helper methods as changing these would require
fixing the tests across all of the different files that use them. In
addition, these helpers already add a code to the Error and we'll
have to discuss how that interacts with the code used by
lib/internal/errors.js. I believe we should convert all users
of errnoException and exceptionWithHostPort in a PR dedicated to
that conversion.

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]

Affected core subsystem(s)

dgram

thefourtheye · 2017-05-09T18:55:34Z

It shows there is a merge conflict in doc/api/errors.md

BridgeAR · 2017-05-09T19:31:46Z

lib/dgram.js

That should probably be a TypeError as well as the other errors thrown in the sendTo function.

I held off changing the types of errors as that may be more breaking than just changing the message. @jasnell what's your take on that front ?

I would go ahead and change the type as appropriate. It's already a breaking change, we might as well rip the bandaid off all at once.

ok will change

mhdawson · 2017-05-09T19:55:32Z

@thefourtheye thanks for pointing that out, rebased.

jasnell · 2017-05-09T21:00:57Z

lib/dgram.js

this can be passed in as ['string', 'falsy']

will update

mhdawson · 2017-05-09T23:37:44Z

@jasnell, @BridgeAR pushed commit to address comments.

BridgeAR · 2017-05-10T00:30:32Z

lib/dgram.js

This should be a TypeError.

This should be a TypeError.
My immediate reaction was to disagree on the grounds that aTypeError means an argument is the wrong type, not missing.
But TypeError is what V8 uses for similar situations, so I'm inclined to agree that Node.js should do the same.
> [0,1,2].forEach()TypeError: undefined is not a function ... >

I think as long as we are consistent across the board, then I'm ok based on the example set by v8

Seems like we already use TypeError in other tests in this case as well. So TypeError is the way to go.

@Trott fixed.

BridgeAR · 2017-05-10T00:30:41Z

lib/dgram.js

This should be a TypeError.

mhdawson · 2017-05-10T12:58:46Z

@BridgeAR thanks for catching those additional TypeErrors, pushed commit to address comment.

mhdawson · 2017-05-10T16:54:12Z

@jasnell all comments so far should be addressed.

mhdawson · 2017-05-10T22:01:21Z

@nodejs/ctc I believe I need a second CTC reviewer since this is semver major.

mhdawson · 2017-05-11T21:29:47Z

@thefourtheye any chance you can take another look ?

fhinkel · 2017-05-23T18:41:05Z

lib/dgram.js

Typo in multicastAddress.

fhinkel · 2017-05-23T18:41:36Z

lib/dgram.js

fhinkel · 2017-05-23T18:42:21Z

lib/dgram.js

Are we usually starting all comments with a capital letter and . at the end?

fhinkel · 2017-05-23T18:44:18Z

test/parallel/test-dgram-membership.js

Typo in Aaddress.

fhinkel · 2017-05-23T18:44:31Z

test/parallel/test-dgram-membership.js

One more 😉

fhinkel

LGTM with the Aaddress typo. Thanks!

mhdawson · 2017-05-23T19:40:55Z

@fhinkel thanks for the review. Will fix those up today.

mhdawson · 2017-05-23T19:58:40Z

@fhinkel pushed commit to address comments. Do you think I should go ahead and land this or wait for the other older PRs to land first. There are sure to be conflicts between them.

fhinkel · 2017-05-23T20:00:59Z

Go ahead and land. We'll rebase the older PRs accordingly.

mhdawson · 2017-05-23T20:51:28Z

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

mhdawson · 2017-05-23T21:37:05Z

See there are already conflicts. Will fix those up.

Covert lib/dgram.js over to using lib/internal/errors.js for generating Errors. See [using-internal-errors.md](https://github.com/nodejs/node/blob/master/doc/guides/using-internal-errors.md) for more details. I have not addressed the cases that use errnoException() and exceptionWithHostPort() helper methods as changing these would require fixing the tests across all of the different files that use them. In addition, these helpers already add a `code` to the Error and we'll have to discuss how that interacts with the `code` used by lib/internal/errors.js. I believe we should convert all users of errnoException and exceptionWithHostPort in a PR dedicated to that conversion.

mhdawson · 2017-05-23T22:10:53Z

rebased and squashed commit for comments.

mhdawson · 2017-05-23T22:13:26Z

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

mhdawson · 2017-05-24T14:21:25Z

Landed as e912c67

Covert lib/dgram.js over to using lib/internal/errors.js for generating Errors. See [using-internal-errors.md](https://github.com/nodejs/node/blob/master/doc/guides/using-internal-errors.md) for more details. I have not addressed the cases that use errnoException() and exceptionWithHostPort() helper methods as changing these would require fixing the tests across all of the different files that use them. In addition, these helpers already add a `code` to the Error and we'll have to discuss how that interacts with the `code` used by lib/internal/errors.js. I believe we should convert all users of errnoException and exceptionWithHostPort in a PR dedicated to that conversion. PR-URL: #12926 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

covert lib/fs.js over to using lib/internal/errors.js i have not addressed the cases that use errnoException(), for reasons described in nodejsGH-12926 - throw the ERR_INVALID_CALLBACK error when the the callback is invalid - replace the ['object', 'string'] with ['string', 'object'] in the error constructor call, to better match the previous err msg in the getOptions() function - add error ERR_VALUE_OUT_OF_RANGE in lib/internal/errors.js, this error is thrown when a numeric value is out of range - document the ERR_VALUE_OUT_OF_RANGE err in errors.md - correct the expected args, in the error thrown in the function fs._toUnixTimestamp() to ['Date', 'time in seconds'] (lib/fs.js) - update the listener error type in the fs.watchFile() function, from Error to TypeError (lib/fs.js) - update errors from ERR_INVALID_OPT_VALUE to ERR_INVALID_ARG_TYPE in the functions fs.ReadStream() and fs.WriteStream(), for the cases of range errors use the new error: ERR_VALUE_OUT_OF_RANGE (lib/fs.js) PR-URL: nodejs#15043 Refs: nodejs#11273 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Covert lib/net.js over to using lib/internal/errors.js Ref: nodejs#11273 I have not addressed the cases that use errnoException(), for reasons described in nodejsGH-12926 - Replace thrown errors in lib/net.js with errors from lib/internal/errors. The ERR_INVALID_OPT_VALUE error have been used in the Server.prototype.listen() method after a discussion in Ref: nodejs#14782 - Update tests according to the above modifications

nodejs-github-bot added dgram Issues and PRs related to the dgram subsystem / UDP. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels May 9, 2017

mhdawson requested a review from jasnell May 9, 2017 18:32

BridgeAR reviewed May 9, 2017
View reviewed changes

mhdawson added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 9, 2017

mhdawson force-pushed the messages1 branch from e82bed3 to 4baeb5fCompare May 9, 2017 19:54

jasnell reviewed May 9, 2017
View reviewed changes

BridgeAR requested changes May 10, 2017
View reviewed changes

BridgeAR approved these changes May 10, 2017
View reviewed changes

jasnell approved these changes May 10, 2017
View reviewed changes