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

util: accept a variable arity of constructors by inherits() #937

Closed
wants to merge 1 commit into from

Conversation

reqshark
Copy link

although the API has long been frozen, and despite stability level 3 and up being off-limits, this should be considered because merit of its usefulness outweighs the need to enforce frozen stability.

instead of doing this:

var util = require('util');
var EventEmitter = require('events').EventEmitter;

util.inherits(Ctor, EventEmitter);
util.inherits(AnotherCtor, EventEmitter);

function Ctor(){}
function AnotherCtor(){}

we ought to do:

require('util').inherits(Ctor, AnotherCtor, require('events').EventEmitter);

function Ctor(){}
function AnotherCtor(){}

@petkaantonov
Copy link
Contributor

I don't like this, it's not obvious what inherits(ClassA, ClassB, ClassC) does at all simply from reading source.

Additionally I think it's very rare to need to do what you are doing (I have never needed it personally) so making it slightly easier is not worth it.

@meandmycode
Copy link

Are there plans with io.js to adopt es6 class syntax at some stage? if there are; it may not be worth adding to just deprecate in favor of syntax.

@tellnes
Copy link
Contributor

tellnes commented Feb 24, 2015

-1 from me also for the reasons outlined by @petkaantonov

@benjamingr
Copy link
Member

Yeah, I agree with Petka here too. People will think it means that Ctor inherits from both ClassB and ClassC in util.inherits(Ctor, ClassB, ClassC)

@gkatsev
Copy link

gkatsev commented Feb 24, 2015

@meandmycode io.js will get es6 class syntax as soon as it gets implemented and released in v8. Whether that syntax ends up getting used in any core code is a different story (probably not).

@benjamingr
Copy link
Member

@gkatsev all util.inherits does is:

exports.inherits = function(ctor, superCtor) {
  ctor.super_ = superCtor;
  ctor.prototype = Object.create(superCtor.prototype, {
    constructor: {
      value: ctor,
      enumerable: false,
      writable: true,
      configurable: true
    }
  });
};

This is a subset of what ES6 classes do, which is roughly (copying what babel does):

var _inherits = function(subClass, superClass) {
    if (typeof superClass !== "function" && superClass !== null) {
        throw new TypeError("Super expression must either be null or a function, not " + typeof superClass);
    }
    subClass.prototype = Object.create(superClass && superClass.prototype, {
        constructor: {
            value: subClass,
            enumerable: false,
            writable: true,
            configurable: true
        }
    });
    if (superClass) subClass.__proto__ = superClass;
};

Overall - very close. Even if it won't be used it core it won't matter much :)

@meandmycode
Copy link

@gkatsev adopt probably not the right word, I should say promote- in that this method would become deprecated from public use anyway.

@domenic
Copy link
Contributor

domenic commented Feb 24, 2015

Wow this is super-confusing, -1.

@chrisdickinson
Copy link
Contributor

Thanks for your contribution! As you mention, this module is locked, so only security fixes, bug fixes, and performance improvements will be considered. Since this patch does not fulfill those requirements, it will not be accepted. Additionally, increased usefulness is a very subjective metric – in this case we have ES2015 classes to look forward to, and it doesn't seem like the new functionality is obvious to most of the reviewers here. Closing this.

@reqshark
Copy link
Author

thanks for the feedback! this suggestion seems rather unpopular. should anyone feel differently it's on npm: https://www.npmjs.com/package/utildot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants