Repository navigation
util.inherits does not establish prototype chain #4179
Description
In inherits.js Object.setPrototypeOf is being called with the wrong parameters.
The reason for me assuming that the wrong parameters are being used is
(REPL)
> function MyError() {}
> util.inherits(MyError, Error);
> MyError
{ [Function: MyError]
super_:
{ [Function: Error]
captureStackTrace: [Function: captureStackTrace],
stackTraceLimit: 10 } }
> Error.isPrototypeOf(MyError);
false
While inheritance sort of works, the prototype chain is actually never established.
> myerr = new MyError()
[Error]
> myerr instanceof MyError
true
> myerr instanceof Error
true
Replacing the existing call to Object.setPrototypeOf() by
Object.setPrototypeOf(ctor, superCtor);
See https://github.com/nodejs/node/blob/master/lib/util.js#L805.
(REPL)
> function MyError() {}
> util.inherits(MyError, Error);
> MyError
[Function: OError]
> Error.isPrototypeOf(MyError);
true
Yet, instanceof will now fail
> myerr = new MyError()
[Error]
> myerr instanceof MyError
true
> myerr instanceof Error
false
When using the new class feature, everything works as expected, though
(REPL)
> class MyError extends Error {}
[Function: MyError]
> Error.isPrototypeOf(MyError)
true
> myerr = new MyError()
[Error]
> myerr instanceof MyError
true
> myerr instanceof Error
true
Activity
[-]util.inherits calls Object.setPrototypeOf with the wrong parameters[/-][+]util.inherits does not establish prototype chain[/+]silkentrance commentedon Dec 7, 2015
See also https://phabricator.babeljs.io/T6765
silkentrance commentedon Dec 7, 2015
See also #3188
bnoordhuis commentedon Dec 8, 2015
That's because inherits() calls
Object.setPrototypeOf(ctor.prototype, superCtor.prototype). It goes all the way back to v0.3.2 so I don't think we can change that without breaking a lot of existing code.Perhaps it's time for inherits() to go gently into that good night and for us to point people to ES6
extends.silkentrance commentedon Dec 8, 2015
Sigh perhaps making it @deprecated would suffice then...
silkentrance commentedon Jan 31, 2016
Reopening as I deem it fit to at least document that shortcoming in the official documentation and deprecate
util.inheritsin favor of new ES2015/harmony features and also in favor of external libraries that get the job done properly.bnoordhuis commentedon Feb 1, 2016
We're open to pull requests that improve the documentation.
As to deprecation, IMO it should start as a documentation-only deprecation that steers people away from using it in new code.
util.inherits()is used in a lot of existing code and is, for the most part, not broken. Printing deprecation warnings is arguably too aggressive / obnoxious.silkentrance commentedon Feb 2, 2016
Well I think that in the past, people did not care too much about
isPrototypeOf, so I think it would be safe to fixutil.inherits. Considering that most users would test recursively for_super. See also the vibejs-subclassof project, which BTW did not gain that much traction 😁As such, I think that with the next release of node the behavior should just be changed and let users just fix their code if they ever managed to screw it up so badly as to rely on that falsely set up prototype chain.
jasnell commentedon Apr 12, 2016
@nodejs/documentation ... not sure about the deprecation here but it would likely be good to include some documentation on the limitations of
util.inheritswith regards to the prototype chain.Knighton910 commentedon Apr 12, 2016
+1
eljefedelrodeodeljefe commentedon Apr 12, 2016
Reading up un this I think the change would actually be possible, no? @bnoordhuis there was this PR in Oct'15 that actually change the from
.create()to.setPrototypeOf(). Though I don't mind the missing chain, the change wouldn't breaking then, or is it?Pointing the
extendsfor real inheritance sounds good to me, though I wouldn't deprecate, since it actually works well.Documentation should only(?): 1. mention that it is overriding the prototype instead appending the chain 2. point to
extendsin order to the chain.bnoordhuis commentedon Apr 12, 2016
For future reference, @eljefedelrodeodeljefe is referring to #3455.
eljefedelrodeodeljefe commentedon Apr 12, 2016
Sorry meant to post the link.
eljefedelrodeodeljefe commentedon May 2, 2016
I am leaning towards class extends as solution for this one. Started a discussion at #6512 for docs.
eljefedelrodeodeljefe commentedon May 9, 2016
FYI this got closed due to a change in docs, see 07c572d. Thx for reporting! It triggered a good conversation and change.
doc: discourage use of util.inherits
doc: discourage use of util.inherits
senocular commentedon Aug 15, 2016
I know this is old, but I stumbled onto this looking for something else, and I think there's a little confusion in the claims of the original issue.
First, as far as I'm concerned, this implementation of inherits is correct, or at least "good", with what we can call "room for improvement". The point of
inheritsis to allow instances of one constructor to inherit members that are available to instances of another (those members stored in the prototype). This is what the current implementation achieves, as seen with the original example with:This is your classic behavior with prototype chain inheritance. Instances of one constructor (
MyError) are able to access members of another (Error) and pass theinstanceofcheck (which compares prototypes).For this example:
It implies that the constructor itself would inherit from the super constructor. Generally this isn't done when setting up inheritance manually, but ES6 happens to do this with
extendsas it allows static members to be inherited. Prior to ES6, I don't think I've ever seen anyone ever do this (plus prior to ES6 andObject.setPrototypeOf, it was only possible going through__proto__which you should feel dirty for even considering using ;P).Now, this behavior is something that can be added on top of the existing behavior if desired. It's not meant to replace it. So "Object.setPrototypeOf is being called with the wrong parameters" is not really the correct way to explain what's going on. If you're expecting the constructor inheritance for static members, then it would be something done along side the existing instance inheritance.
joshxyzhimself commentedon Apr 28, 2020
@senocular does that mean that adding
below https://github.com/nodejs/node/blob/master/lib/util.js#L168
makes it work exactly like es6
extends?addaleax commentedon Apr 28, 2020
@joshxyzhimself
extendsdoesn’t definesuper_, but otherwise, yes.Use class syntax and extends keyword instead of util.inherits (#16)