Skip to content
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

Fail verbosely on invalid use of util.inherits #1240

Closed
wants to merge 4 commits into from

Conversation

connor4312
Copy link
Contributor

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!

@mscdex mscdex added the util Issues and PRs related to the built-in util module. label Mar 23, 2015
*/
exports.inherits = function(ctor, superCtor) {

if (isNullOrUndefined(ctor))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the codebase, the util.isXXX() functions have been phased out since 6ac8bdc, could you change them here and below to an inline alternative?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done so

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.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is over 80 lines (make jslint), could you somehow fix that?

*/
exports.inherits = function(ctor, superCtor) {

if (ctor === undefined || ctor === null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That in turn triggers a jshint error. Wasn't feeling ballsy enough to add a jshint ignore line in my first PR :P

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes == null is exactly the same as === null || === undefined

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

io.js uses the closure linter, so a jshint ignore line wouldn't be necessary, unless it also does the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does. I originally used == null, but that triggered an error.

@brendanashworth
Copy link
Contributor

This LGTM as is, I left the === vs == call to you. Seeking another collaborator though.

Note to the future generation: before this, we have zero tests for util.inherits?

@petkaantonov
Copy link
Contributor

LGTM

@connor4312
Copy link
Contributor Author

Note to the future generation: before this, we have zero tests for util.inherits?

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.

brendanashworth pushed a commit that referenced this pull request Mar 23, 2015
PR-URL: #1240
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
Reviewed-By: Petka Antonov <petka_antonov@hotmail.com>
@brendanashworth
Copy link
Contributor

Thanks Connor, this has been merged in 849319a.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants