-
Notifications
You must be signed in to change notification settings - Fork 30k
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
util: fixes type in argument type validation error #25103
util: fixes type in argument type validation error #25103
Conversation
Test needs updating for this change. 12:54:18 not ok 2000 parallel/test-util-inherits
12:54:18 ---
12:54:18 duration_ms: 0.136
12:54:18 severity: fail
12:54:18 exitcode: 1
12:54:18 stack: |-
12:54:18 /Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/test/common/index.js:580
12:54:18 throw new assert.AssertionError({
12:54:18 ^
12:54:18
12:54:18 AssertionError [ERR_ASSERTION]: Expected "actual" to be reference-equal to "expected":
12:54:18 + actual - expected
12:54:18
12:54:18 Comparison {
12:54:18 code: 'ERR_INVALID_ARG_TYPE',
12:54:18 + message: 'The "superCtor.prototype" property must be of type Object. Received type undefined',
12:54:18 - message: 'The "superCtor.prototype" property must be of type Function. Received type undefined',
12:54:18 type: [Function: TypeError]
12:54:18 }
12:54:18 at new AssertionError (internal/assert.js:396:11)
12:54:18 at Object.innerFn (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/test/common/index.js:580:15)
12:54:18 at expectedException (assert.js:568:19)
12:54:18 at expectsError (assert.js:663:16)
12:54:18 at Function.throws (assert.js:694:3)
12:54:18 at Object.expectsError (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/test/common/index.js:592:12)
12:54:18 at Object.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/test/parallel/test-util-inherits.js:86:8)
12:54:18 at Module._compile (internal/modules/cjs/loader.js:718:30)
12:54:18 at Object.Module._extensions..js (internal/modules/cjs/loader.js:729:10)
12:54:18 at Module.load (internal/modules/cjs/loader.js:617:32)
12:54:18 ... |
Ping @aoberoi |
ca22693
to
4ce3523
Compare
4ce3523
to
fe1e8ea
Compare
@@ -317,7 +317,7 @@ function inherits(ctor, superCtor) { | |||
|
|||
if (superCtor.prototype === undefined) { | |||
throw new ERR_INVALID_ARG_TYPE('superCtor.prototype', | |||
'Function', superCtor.prototype); | |||
'Object', superCtor.prototype); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Object', superCtor.prototype); | |
['Object', 'Function'], superCtor.prototype); |
ECMAScript does not distinguish objects and functions but users do. Both will be accepted and all other inputs (besides null) throw.
I personally would either say we should remove the check to align the error message with the one for primitives or we should check for those as well. However, that would be semver-major and this function is already deprecated, so there's probably not much point in doing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BridgeAR thanks for the review! i'm excited to land a commit 😄
I don't agree with the suggested change because the value of superCtor.prototype
must be an object. Sure, a function is an object so that works, but its not the function-ness of this value that is important, it is the object-ness (the intention is that this value's properties become available in the prototype chain, that's an object-y aspect of this value). I could apply your reasoning here and say that the suggested change should include Array
, RegExp
, etc, since they are also all objects and are equally meaningful here as Function
, but I think you see that it wouldn't make the error any more meaningful to include those.
Also, I'm not sure which function you mean is deprecated. From the public docs, I don't see any deprecation for util.inherits()
. Do you mean that ERR_INVALID_ARG_TYPE()
is deprecated? If so, what is the more favorable alternative?
Thanks for your help!
Ping @Trott |
I see a couple build errors, but I don't think they are related to this change. Those seem to both be related to Worker Threads. Are they known to be flaky? Possibly related: #21246 |
Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19927/ ✅ |
Ping @BridgeAR |
Landed in eb5aab2, thanks for the PR! |
PR-URL: #25103 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#25103 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: #25103 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#25103 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: #25103 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: #25103 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: #25103 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
The argument validation for
superCtor
should not output thatsuperCtor.prototype
must be a Function, but rather it must be an Object.
Checklist