internal/errors: improve ERR_INVALID_ARG_TYPE #13730

BridgeAR · 2017-06-17T00:33:34Z

The error message might be misleading if a object property
was the issue and not the argument itself.

Fix this by checking if a argument or a property is passed
to the handler function.

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)

internal/errors

refack

👍 FWIW
(need 2 CTC members' approval for semver-majors)

TimothyGu

+1 for semver-patch. IIRC the point of these new error objects is that changes in the error message can be done without semver-major because of the existence of code.

refack · 2017-06-17T04:07:29Z

+1 for semver-patch. IIRC the point of these new error objects is that changes in the error message can be done without semver-major because of the existence of code.

Anyone with more experience then me, feel free to downgrade semver level.

lpinca · 2017-06-17T07:31:31Z

test/parallel/test-process-hrtime.js

@@ -35,7 +35,7 @@ validateTuple(process.hrtime(tuple));
 const invalidHrtimeArgument = common.expectsError({
  code: 'ERR_INVALID_ARG_TYPE',
  type: TypeError,
-  message: 'The "process.hrtime()" argument must be of type Array'
+  message: 'The "process.hrtime()" property must be of type Array'


I think this is still misleading.

Using the word argument here was correct. Please wait for a decision to #13739, but this should not be changed.

tniessen · 2017-06-17T12:04:07Z

Effectively blocked by #13739, see #13730 (comment)

tniessen

I would like to merge #13739 in about 48 hours. In order to start CI for your PR afterwards, I need you to revert your modifications to test/parallel/test-process-hrtime.js. Take your time, there is no need to hurry (we cannot merge this before #13739 is landed and two @nodejs/ctc members still need to approve this as long as it is semver-major).

tniessen · 2017-06-17T13:43:32Z

test/parallel/test-process-hrtime.js

@@ -35,7 +35,7 @@ validateTuple(process.hrtime(tuple));
 const invalidHrtimeArgument = common.expectsError({
  code: 'ERR_INVALID_ARG_TYPE',
  type: TypeError,
-  message: 'The "process.hrtime()" argument must be of type Array'
+  message: 'The "process.hrtime()" property must be of type Array'


This change should be reverted, see above.

mscdex · 2017-06-17T14:21:54Z

@tniessen I believe we still try to give 72 hours for review during weekends instead of the 48 hours given during the week.

tniessen · 2017-06-17T14:30:11Z

@mscdex My bad, I scheduled it for Tuesday... You are right, that's 72 hours, not 48.

BridgeAR · 2017-06-20T20:11:08Z

In addition to the current change I would like to also check if the provided type is actually a primitive or a class (either with a whitelist of primitives or by checking if the first letter is upper case) and change the error message accordingly. If it's a class we could write instance of instead of type of and also print the provided constructor name in case the value was provided and it is of type object. The one in "one of" could also be removed if I'm not mistaken.

function test (opts) {
  if (!isUint8Array(opts.prop))
    throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'opts.prop' ['Buffer', 'Uint8Array'], opts.prop)
}

test({ prop: [] })
// TypeError [ERR_INVALID_ARG_TYPE]: The "opts.prop" property must be instance of Buffer or Uint8Array. Received instance of Array.
// instead of
// TypeError [ERR_INVALID_ARG_TYPE]: The "opts.prop" property must be one of type Buffer or Uint8Array. Received type object

test({ prop: 'string' })
// TypeError [ERR_INVALID_ARG_TYPE]: The "opts.prop" property must be instance of Buffer or Uint8Array. Received type string.
// instead of
// TypeError [ERR_INVALID_ARG_TYPE]: The "opts.prop" property must be one of type Buffer or Uint8Array. Received type string

Should I just update the PR or open another one for that?

Neither the current change nor the proposed one should be semver major though as the error codes do not change and the messages are meant to change.

jasnell · 2017-06-20T20:17:07Z

This does not need to be semver-major.

tniessen · 2017-06-20T20:46:43Z

@BridgeAR I would prefer a separate PR to discuss such changes.

Please rebase this on master and remove any modifications to test/parallel/test-process-hrtime.js (basically squash the second commit). CI won't run cleanly as it cannot automaticall merge the first commit.

refack · 2017-06-20T21:01:35Z

Learned a new thing about error message semverity.
Ping @tniessen should this be un-blocked?

The error message might be misleading if a object property was the issue and not the argument itself. Fix this by checking if a argument or a property is passed to the handler function.

BridgeAR · 2017-06-20T22:01:50Z

@tniessen done

refack · 2017-06-20T22:07:47Z

Pre land CI: https://ci.nodejs.org/job/node-test-commit/10693/

The error message might be misleading if an object property was the issue and not the argument itself. Fix this by checking if a argument or a property is passed to the handler function. PR-URL: nodejs#13730 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

refack · 2017-06-22T17:56:04Z

Landed in 3e17884

mscdex · 2017-06-22T20:37:37Z

@jasnell Can you explain why it's not semver-major?

jasnell · 2017-06-22T20:46:52Z

It does not change any error types or the error code. The changes are limited to the error message and only in certain cases. Per the guidelines for the new internal/errors, changes to the message are not semver-major once the error has been migrated to use codes.

mscdex · 2017-06-22T21:01:58Z

@jasnell Why treat the error type specially (assuming you're referring to use of Error, TypeError, RangeError, etc.) and not the message? I would think users could be explicitly checking both?

Also, I'm kind of surprised we're not giving any grace period for users to switch after the new errors system is rolled out (everywhere).

refack · 2017-06-22T21:11:42Z

Also, I'm kind of surprised we're not giving any grace period for users to switch after the new errors system is rolled out (everywhere).

I'm assuming since this changed only internal errors, and the change over to internal errors is semver-major the impact is mitigated.

mscdex · 2017-06-22T21:16:51Z

I'm assuming since this changed only internal errors

@refack I don't understand what you mean by 'internal' errors. This PR changes error messages received by end users of node.

refack · 2017-06-22T21:26:48Z

I don't understand what you mean by 'internal' errors

The new errors that must include .code as tracked by #11273. Those errors' messages were already changed in a semver-major way.

mscdex · 2017-06-22T21:32:04Z

@refack Typically the messages aren't changed (initially) but what I said is still relevant:

Also, I'm kind of surprised we're not giving any grace period for users to switch after the new errors system is rolled out (everywhere).

refack · 2017-06-23T21:50:52Z

Temporarily "don't land"ing this until semverity is decided.

nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Jun 17, 2017

refack added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 17, 2017

refack approved these changes Jun 17, 2017

View reviewed changes

TimothyGu approved these changes Jun 17, 2017

View reviewed changes

lpinca approved these changes Jun 17, 2017

View reviewed changes

lpinca reviewed Jun 17, 2017

View reviewed changes

tniessen added the blocked PRs that are blocked by other issues or PRs. label Jun 17, 2017

tniessen requested changes Jun 17, 2017

View reviewed changes

mhdawson approved these changes Jun 20, 2017

View reviewed changes

jasnell removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 20, 2017

tniessen approved these changes Jun 20, 2017

View reviewed changes

tniessen removed the blocked PRs that are blocked by other issues or PRs. label Jun 20, 2017

refack self-assigned this Jun 20, 2017

refack mentioned this pull request Jun 20, 2017

internal: use internal/errors #13829

Closed

3 tasks

internal/errors: improve ERR_INVALID_ARG_TYPE

adc7c88

The error message might be misleading if a object property was the issue and not the argument itself. Fix this by checking if a argument or a property is passed to the handler function.

BridgeAR force-pushed the property-argument branch from 5bde256 to adc7c88 Compare June 20, 2017 22:01

BridgeAR mentioned this pull request Jun 21, 2017

errors: improve invalid arg type #13834

Merged

3 tasks

refack closed this Jun 22, 2017

refack added the dont-land-on-v8.x label Jun 23, 2017

This was referenced Jun 25, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-06-28 nodejs/CTC#148

Closed

meta: semver impact of Error messages #13937

Closed

targos added semver-major PRs that contain breaking changes and should be released in the next major version. and removed dont-land-on-v8.x labels Aug 30, 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

internal/errors: improve ERR_INVALID_ARG_TYPE #13730

internal/errors: improve ERR_INVALID_ARG_TYPE #13730

BridgeAR commented Jun 17, 2017

refack left a comment

TimothyGu left a comment

refack commented Jun 17, 2017

lpinca Jun 17, 2017

tniessen Jun 17, 2017

tniessen commented Jun 17, 2017

tniessen left a comment

tniessen Jun 17, 2017 •

edited

Loading

mscdex commented Jun 17, 2017

tniessen commented Jun 17, 2017

BridgeAR commented Jun 20, 2017

jasnell commented Jun 20, 2017

tniessen commented Jun 20, 2017 •

edited

Loading

refack commented Jun 20, 2017

BridgeAR commented Jun 20, 2017

refack commented Jun 20, 2017

refack commented Jun 22, 2017

mscdex commented Jun 22, 2017 •

edited

Loading

jasnell commented Jun 22, 2017

mscdex commented Jun 22, 2017

refack commented Jun 22, 2017

mscdex commented Jun 22, 2017 •

edited

Loading

refack commented Jun 22, 2017

mscdex commented Jun 22, 2017

refack commented Jun 23, 2017

internal/errors: improve ERR_INVALID_ARG_TYPE #13730

internal/errors: improve ERR_INVALID_ARG_TYPE #13730

Conversation

BridgeAR commented Jun 17, 2017

Checklist

Affected core subsystem(s)

refack left a comment

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

refack commented Jun 17, 2017

lpinca Jun 17, 2017

Choose a reason for hiding this comment

tniessen Jun 17, 2017

Choose a reason for hiding this comment

tniessen commented Jun 17, 2017

tniessen left a comment

Choose a reason for hiding this comment

tniessen Jun 17, 2017 • edited Loading

Choose a reason for hiding this comment

mscdex commented Jun 17, 2017

tniessen commented Jun 17, 2017

BridgeAR commented Jun 20, 2017

jasnell commented Jun 20, 2017

tniessen commented Jun 20, 2017 • edited Loading

refack commented Jun 20, 2017

BridgeAR commented Jun 20, 2017

refack commented Jun 20, 2017

refack commented Jun 22, 2017

mscdex commented Jun 22, 2017 • edited Loading

jasnell commented Jun 22, 2017

mscdex commented Jun 22, 2017

refack commented Jun 22, 2017

mscdex commented Jun 22, 2017 • edited Loading

refack commented Jun 22, 2017

mscdex commented Jun 22, 2017

refack commented Jun 23, 2017

tniessen Jun 17, 2017 •

edited

Loading

tniessen commented Jun 20, 2017 •

edited

Loading

mscdex commented Jun 22, 2017 •

edited

Loading

mscdex commented Jun 22, 2017 •

edited

Loading