util: fixes type in argument type validation error#25103
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added the util 
lpinca commented Dec 18, 2018
Trott commented Dec 19, 2018
Test needs updating for this change. 12:54:18 not ok 2000 parallel/test-util-inherits12:54:18 ---12:54:18 duration_ms: 0.13612:54:18 severity: fail12:54:18 exitcode: 112:54:18 stack: |-12:54:18 /Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/test/common/index.js:58012: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 - expected12: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 ... |
BridgeAR commented Dec 27, 2018
Ping @aoberoi |
aoberoiforce-pushed the fix-util.inherits-arg-type-error-message branch from ca22693 to 4ce3523Compareaoberoiforce-pushed the fix-util.inherits-arg-type-error-message branch from 4ce3523 to fe1e8eaCompareaoberoi commented Dec 28, 2018
| if(superCtor.prototype===undefined){ | ||
| thrownewERR_INVALID_ARG_TYPE('superCtor.prototype', | ||
| 'Function',superCtor.prototype); | ||
| 'Object',superCtor.prototype); |
There was a problem hiding this comment.
| '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.
@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!
aoberoi commented Jan 3, 2019
Ping @Trott |
Trott commented Jan 4, 2019
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/ ✅ |
addaleax added the author ready 
addaleax commented Jan 7, 2019
Ping @BridgeAR |
addaleax commented Jan 14, 2019
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]>
6 participants
The argument validation for
superCtorshould not output thatsuperCtor.prototypemust be a Function, but rather it must be an Object.
Checklist