Skip to content

Remove util.inherits usage internally? #24395

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
joyeecheung opened this issue Nov 16, 2018 · 5 comments
Closed

Remove util.inherits usage internally? #24395

joyeecheung opened this issue Nov 16, 2018 · 5 comments
Labels
good first issue Issues that are suitable for first-time contributors. util Issues and PRs related to the built-in util module.

Comments

@joyeecheung
Copy link
Member

Currently, util.inherits does this:

node/lib/util.js

Lines 300 to 318 in e1aa730

function inherits(ctor, superCtor) {
if (ctor === undefined || ctor === null)
throw new ERR_INVALID_ARG_TYPE('ctor', 'Function', ctor);
if (superCtor === undefined || superCtor === null)
throw new ERR_INVALID_ARG_TYPE('superCtor', 'Function', superCtor);
if (superCtor.prototype === undefined) {
throw new ERR_INVALID_ARG_TYPE('superCtor.prototype',
'Function', superCtor.prototype);
}
Object.defineProperty(ctor, 'super_', {
value: superCtor,
writable: true,
configurable: true
});
Object.setPrototypeOf(ctor.prototype, superCtor.prototype);
}

Most of the code seems unnecessary for our internal use. Essentially we can just replace all the internal util.inherits usage with Object.setPrototypeOf(ctor.prototype, superCtor.prototype)

Unless someone is relying on that ctor._super property existing on our internal types..

Any thoughts?

@joyeecheung joyeecheung added the util Issues and PRs related to the built-in util module. label Nov 16, 2018
@refack
Copy link
Contributor

refack commented Nov 16, 2018

I'm +1 on writing a PR and running CitGM on it, then have it in nightlies go get some feedback. Otherwise we won't know.
(Ohh, and run a query on Gzemnid)

BridgeAR added a commit to BridgeAR/node that referenced this issue Dec 2, 2018
This switches all `util.inherits()` calls to use
`Object.setPrototypeOf()` instead. In fact, `util.inherits()` is
mainly a small wrapper around exactly this function while adding
the `_super` property on the object as well.

Refs: nodejs#24395
addaleax pushed a commit that referenced this issue Dec 5, 2018
This switches all `util.inherits()` calls to use
`Object.setPrototypeOf()` instead. In fact, `util.inherits()` is
mainly a small wrapper around exactly this function while adding
the `_super` property on the object as well.

Refs: #24395

PR-URL: #24755
Refs: #24395
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
refack pushed a commit to refack/node that referenced this issue Jan 14, 2019
This switches all `util.inherits()` calls to use
`Object.setPrototypeOf()` instead. In fact, `util.inherits()` is
mainly a small wrapper around exactly this function while adding
the `_super` property on the object as well.

Refs: nodejs#24395

PR-URL: nodejs#24755
Refs: nodejs#24395
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@joyeecheung
Copy link
Member Author

This is still open, can probably be used as code-and-learn tasks

@targos targos added the good first issue Issues that are suitable for first-time contributors. label Mar 9, 2019
@ghost
Copy link

ghost commented Mar 12, 2019

If the issue is still open, I'd be interested in looking into this?

@cjihrig
Copy link
Contributor

cjihrig commented Mar 12, 2019

@peanutenthusiast check out #26546 for an updated version of this issue.

@BridgeAR
Copy link
Member

Closing as resolved. There is no usage internal usage left in core.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
good first issue Issues that are suitable for first-time contributors. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

No branches or pull requests

5 participants