Fail verbosely on invalid use of util.inherits #1240
Conversation
| */ | ||
| exports.inherits = function(ctor, superCtor) { | ||
|
|
||
| if (isNullOrUndefined(ctor)) |
There was a problem hiding this comment.
In the codebase, the util.isXXX() functions have been phased out since 6ac8bdc, could you change them here and below to an inline alternative?
| if (ctor === undefined || ctor === null) | ||
| throw new TypeError('The constructor to `inherits` must not be null.'); | ||
| if (superCtor === undefined || superCtor === null) | ||
| throw new TypeError('The super constructor to `inherits` must not be null.'); |
There was a problem hiding this comment.
This line is over 80 lines (make jslint), could you somehow fix that?
| */ | ||
| exports.inherits = function(ctor, superCtor) { | ||
|
|
||
| if (ctor === undefined || ctor === null) |
There was a problem hiding this comment.
Do you have any opinions on loose equality here and below, i.e. ctor == null? I think it would behave the same way, but that's your call to use or not
There was a problem hiding this comment.
That in turn triggers a jshint error. Wasn't feeling ballsy enough to add a jshint ignore line in my first PR :P
There was a problem hiding this comment.
Yes == null is exactly the same as === null || === undefined
There was a problem hiding this comment.
io.js uses the closure linter, so a jshint ignore line wouldn't be necessary, unless it also does the same.
There was a problem hiding this comment.
It does. I originally used == null, but that triggered an error.
|
This LGTM as is, I left the Note to the future generation: before this, we have zero tests for |
|
LGTM |
Yea, I noticed that too. A bunch of other tests use util.inherits, so any error would probably manifest itself... but it's good practice to test it directly. |
PR-URL: #1240 Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com> Reviewed-By: Petka Antonov <petka_antonov@hotmail.com>
|
Thanks Connor, this has been merged in |
Previously, passing an undefined or invalid constructor into inherits failed with an error related to the implementation of the function -- one that may not be immediately obvious to a consumer. I think that it is better say exactly what's wrong!