New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dgram: convert to using internal/errors #12926

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@mhdawson
Member

mhdawson commented May 9, 2017

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

@mhdawson mhdawson requested a review from jasnell May 9, 2017

@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

thefourtheye May 9, 2017

Contributor

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

Contributor

thefourtheye commented May 9, 2017

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

Show outdated Hide outdated lib/dgram.js
if (typeof offset !== 'number' || typeof length !== 'number')
throw new Error('Send takes "offset" and "length" as args 2 and 3');
if (typeof offset !== 'number') {
throw new errors.Error('ERR_INVALID_ARG_TYPE', 'offset', 'number');

This comment has been minimized.

@BridgeAR

BridgeAR May 9, 2017

Member

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

@BridgeAR

BridgeAR May 9, 2017

Member

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

This comment has been minimized.

@mhdawson

mhdawson May 9, 2017

Member

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 ?

@mhdawson

mhdawson May 9, 2017

Member

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 ?

This comment has been minimized.

@jasnell

jasnell May 9, 2017

Member

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.

@jasnell

jasnell May 9, 2017

Member

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.

This comment has been minimized.

@mhdawson

mhdawson May 9, 2017

Member

ok will change

@mhdawson

mhdawson May 9, 2017

Member

ok will change

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 9, 2017

Member

@thefourtheye thanks for pointing that out, rebased.

Member

mhdawson commented May 9, 2017

@thefourtheye thanks for pointing that out, rebased.

Show outdated Hide outdated lib/dgram.js
'string or falsy');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'address',
'string or falsy');

This comment has been minimized.

@jasnell

jasnell May 9, 2017

Member

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

@jasnell

jasnell May 9, 2017

Member

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

This comment has been minimized.

@mhdawson

mhdawson May 9, 2017

Member

will update

@mhdawson

mhdawson May 9, 2017

Member

will update

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 9, 2017

Member

@jasnell, @BridgeAR pushed commit to address comments.

Member

mhdawson commented May 9, 2017

@jasnell, @BridgeAR pushed commit to address comments.

Show outdated Hide outdated lib/dgram.js
@@ -543,7 +563,7 @@ Socket.prototype.addMembership = function(multicastAddress,
this._healthCheck();
if (!multicastAddress) {
throw new Error('multicast address must be specified');
throw new errors.Error('ERR_MISSING_ARGS', 'multicastAaddress');

This comment has been minimized.

@BridgeAR

BridgeAR May 10, 2017

Member

This should be a TypeError.

@BridgeAR

BridgeAR May 10, 2017

Member

This should be a TypeError.

This comment has been minimized.

@Trott

Trott May 10, 2017

Member

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
 ...
>
@Trott

Trott May 10, 2017

Member

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
 ...
>

This comment has been minimized.

@mhdawson

mhdawson May 10, 2017

Member

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

@mhdawson

mhdawson May 10, 2017

Member

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

This comment has been minimized.

@mhdawson

mhdawson May 10, 2017

Member

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

@mhdawson

mhdawson May 10, 2017

Member

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

This comment has been minimized.

@mhdawson

mhdawson May 10, 2017

Member

Will Fix.

@mhdawson

mhdawson May 10, 2017

Member

Will Fix.

This comment has been minimized.

@mhdawson

mhdawson May 12, 2017

Member

@Trott fixed.

@mhdawson
Show outdated Hide outdated lib/dgram.js
@@ -558,7 +578,7 @@ Socket.prototype.dropMembership = function(multicastAddress,
this._healthCheck();
if (!multicastAddress) {
throw new Error('multicast address must be specified');
throw new errors.Error('ERR_MISSING_ARGS', 'multicastAaddress');

This comment has been minimized.

@BridgeAR

BridgeAR May 10, 2017

Member

This should be a TypeError.

@BridgeAR

BridgeAR May 10, 2017

Member

This should be a TypeError.

This comment has been minimized.

@mhdawson

mhdawson May 10, 2017

Member

Will Fix.

@mhdawson

mhdawson May 10, 2017

Member

Will Fix.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 10, 2017

Member

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

Member

mhdawson commented May 10, 2017

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

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 10, 2017

Member

@jasnell all comments so far should be addressed.

Member

mhdawson commented May 10, 2017

@jasnell all comments so far should be addressed.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 10, 2017

Member

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

Member

mhdawson commented May 10, 2017

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

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 11, 2017

Member

@thefourtheye any chance you can take another look ?

Member

mhdawson commented May 11, 2017

@thefourtheye any chance you can take another look ?

Show outdated Hide outdated lib/dgram.js
@@ -543,7 +563,7 @@ Socket.prototype.addMembership = function(multicastAddress,
this._healthCheck();
if (!multicastAddress) {
throw new Error('multicast address must be specified');
throw new errors.TypeError('ERR_MISSING_ARGS', 'multicastAaddress');

This comment has been minimized.

@fhinkel

fhinkel May 23, 2017

Member

Typo in multicastAddress.

@fhinkel

fhinkel May 23, 2017

Member

Typo in multicastAddress.

Show outdated Hide outdated lib/dgram.js
@@ -558,7 +578,7 @@ Socket.prototype.dropMembership = function(multicastAddress,
this._healthCheck();
if (!multicastAddress) {
throw new Error('multicast address must be specified');
throw new errors.TypeError('ERR_MISSING_ARGS', 'multicastAaddress');

This comment has been minimized.

@fhinkel

fhinkel May 23, 2017

Member

Same typo.

@fhinkel

fhinkel May 23, 2017

Member

Same typo.

Show outdated Hide outdated lib/dgram.js
if (!this._handle)
throw new Error('Not running'); // error message from dgram_legacy.js
if (!this._handle) {
// error message from dgram_legacy.js

This comment has been minimized.

@fhinkel

fhinkel May 23, 2017

Member

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

@fhinkel

fhinkel May 23, 2017

Member

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

Show outdated Hide outdated test/parallel/test-dgram-membership.js
}, common.expectsError({
code: 'ERR_MISSING_ARGS',
type: TypeError,
message: /^The "multicastAaddress" argument must be specified$/

This comment has been minimized.

@fhinkel

fhinkel May 23, 2017

Member

Typo in Aaddress.

@fhinkel

fhinkel May 23, 2017

Member

Typo in Aaddress.

Show outdated Hide outdated test/parallel/test-dgram-membership.js
}, common.expectsError({
code: 'ERR_MISSING_ARGS',
type: TypeError,
message: /^The "multicastAaddress" argument must be specified$/

This comment has been minimized.

@fhinkel

fhinkel May 23, 2017

Member

One more 😉

@fhinkel

fhinkel May 23, 2017

Member

One more 😉

@fhinkel

LGTM with the Aaddress typo. Thanks!

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 23, 2017

Member

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

Member

mhdawson commented May 23, 2017

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

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 23, 2017

Member

@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.

Member

mhdawson commented May 23, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel May 23, 2017

Member

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

Member

fhinkel commented May 23, 2017

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

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 23, 2017

Member

See there are already conflicts. Will fix those up.

Member

mhdawson commented May 23, 2017

See there are already conflicts. Will fix those up.

dgram: convert to using internal/errors
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

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 23, 2017

Member

rebased and squashed commit for comments.

Member

mhdawson commented May 23, 2017

rebased and squashed commit for comments.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 24, 2017

Member

Landed as e912c67

Member

mhdawson commented May 24, 2017

Landed as e912c67

@mhdawson mhdawson closed this May 24, 2017

mhdawson added a commit that referenced this pull request May 24, 2017

dgram: convert to using internal/errors
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 <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben.bridgewater@fintura.de>

jasnell added a commit that referenced this pull request May 24, 2017

dgram: convert to using internal/errors
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 <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben.bridgewater@fintura.de>

jasnell added a commit that referenced this pull request May 28, 2017

dgram: convert to using internal/errors
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 <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben.bridgewater@fintura.de>

@jasnell jasnell referenced this pull request May 28, 2017

Closed

8.0.0 Release Proposal #12220

@jasnell jasnell referenced this pull request Jun 20, 2017

Closed

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

78 of 80 tasks complete

@mhdawson mhdawson deleted the mhdawson:messages1 branch Jun 28, 2017

pmatzavin added a commit to pmatzavin/node that referenced this pull request Aug 11, 2017

net: Convert to using internal/errors
Covert lib/net.js over to using lib/internal/errors.js

Ref: #11273

I have not addressed the cases that use errnoException(),
for reasons described in GH-12926

- Add ERR_NET_INVALID_LISTEN_ARG error in lib/internal/errors.js
- Document the ERR_NET_INVALID_LISTEN_ARG error in errors.md
- Replace thrown errors in lib/net.js
  with errors from  lib/internal/errors
- Update tests according to the above modification

pmatzavin added a commit to pmatzavin/node that referenced this pull request Aug 11, 2017

lib/net: Convert to using internal/errors
Covert lib/net.js over to using lib/internal/errors.js

Ref: #11273

I have not addressed the cases that use errnoException(),
for reasons described in GH-12926

- Add ERR_NET_INVALID_LISTEN_ARG error in lib/internal/errors.js
- Document the ERR_NET_INVALID_LISTEN_ARG error in errors.md
- Replace thrown errors in lib/net.js
  with errors from  lib/internal/errors
- Update tests according to the above modification

@pmatzavin pmatzavin referenced this pull request Aug 12, 2017

Closed

lib/net: Convert to using internal/errors #14782

4 of 4 tasks complete

pmatzavin added a commit to pmatzavin/node that referenced this pull request Aug 20, 2017

lib/net: Convert to using internal/errors
Covert lib/net.js over to using lib/internal/errors.js

Ref: #11273

I have not addressed the cases that use errnoException(),
for reasons described in GH-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: #14782
- Update tests according to the above modifications

pmatzavin added a commit to pmatzavin/node that referenced this pull request Aug 26, 2017

Covert lib/fs.js over to using lib/internal/errors.js
Ref: #11273

I have not addressed the cases that use errnoException(),
for reasons described in GH-12926

pmatzavin added a commit to pmatzavin/node that referenced this pull request Aug 26, 2017

lib/fs: Convert to using internal/errors
Covert lib/fs.js over to using lib/internal/errors.js

Ref: #11273

I have not addressed the cases that use errnoException(),
for reasons described in GH-12926

pmatzavin added a commit to pmatzavin/node that referenced this pull request Aug 26, 2017

lib/fs: convert to using internal/errors
covert lib/fs.js over to using lib/internal/errors.js

ref: #11273

i have not addressed the cases that use errnoException(),
for reasons described in GH-12926

@pmatzavin pmatzavin referenced this pull request Aug 26, 2017

Merged

lib/fs: convert to using internal/errors #15043

4 of 4 tasks complete

refack added a commit to pmatzavin/node that referenced this pull request Aug 31, 2017

lib/fs: convert to using internal/errors
covert lib/fs.js over to using lib/internal/errors.js
i have not addressed the cases that use errnoException(),
for reasons described in GH-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 <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

refack added a commit to refack/node that referenced this pull request Aug 31, 2017

errors: convert 'fs'
covert lib/fs.js over to using lib/internal/errors.js
i have not addressed the cases that use errnoException(),
for reasons described in GH-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 <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

addaleax added a commit to addaleax/node that referenced this pull request Sep 13, 2017

errors: convert 'fs'
covert lib/fs.js over to using lib/internal/errors.js
i have not addressed the cases that use errnoException(),
for reasons described in GH-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 <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

pmatzavin added a commit to pmatzavin/node that referenced this pull request Sep 19, 2017

lib/net: Convert to using internal/errors
Covert lib/net.js over to using lib/internal/errors.js

Ref: #11273

I have not addressed the cases that use errnoException(),
for reasons described in GH-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: #14782
- Update tests according to the above modifications

jasnell added a commit to pmatzavin/node that referenced this pull request Sep 25, 2017

lib/net: Convert to using internal/errors
Covert lib/net.js over to using lib/internal/errors.js

Ref: #11273

I have not addressed the cases that use errnoException(),
for reasons described in GH-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: #14782
- Update tests according to the above modifications
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment