-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
doc: discourage use of util.inherits; point to es6 extends #6514
doc: discourage use of util.inherits; point to es6 extends #6514
Conversation
util.inherits breaks the prototype chain. A fix does not seem useful, since ES6 extends provides language level support for the same functionality. This commit starts fasing out mentions of the method. Fixes: #4179
}); | ||
myEmitter.emit('event'); | ||
``` | ||
uses the ES6 classes. It is however possible to use traditional Node.js style |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'the ES6 classes'? I'd drop the reference to util.inherits()
entirely, by the way.
@bnoordhuis addressed your feedback in 28332bb with a question. Dragging in @jasnell here with regards to #6512 |
var buf = Buffer.from(str, 'ascii'); | ||
this.push(buf); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still a semicolon here. If there are more, I won't point them out but please fix them. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed that one. I hope found them all now. Note to myself: add doc linting some time.
@bnoordhuis does your #6545 block this here? Otherwise LGTY @jasnell @targos @bnoordhuis ? |
LGTM at a quick glance, looks like you addressed all the comments. |
@@ -126,6 +126,10 @@ util.format(1, 2, 3); // '1 2 3' | |||
|
|||
## util.inherits(constructor, superConstructor) | |||
|
|||
_Note: usage of util.inherits() is discouraged. Please use the ES6 `class` and | |||
`extends` keywords to get language level inheritance support. Also note that | |||
the two styles are semantically incompatible._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is the wrong place, but a link to explanation of semantically incompatible
would be helpful if there is a good one, and we are ok with external links. Perhaps even just a link in the PR so it can be found by tracing back to this PR.
@sam-github amended the link. |
LGTM |
@sam-github LGTY? |
LGTM |
@nodejs/collaborators we are discouraging the use of |
If you can force-push fast, go for it. :) |
util.inherits breaks the prototype chain. A fix does not seem useful, since ES6 extends provides language level support for the same functionality. This commit starts fasing out mentions of the method. Fixes: #6512 Fixes: #4179 PR-URL: #6514 Reviewed-By: James Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
9daf4a2 then |
util.inherits breaks the prototype chain. A fix does not seem useful, since ES6 extends provides language level support for the same functionality. This commit starts fasing out mentions of the method. Fixes: #6512 Fixes: #4179 PR-URL: #6514 Reviewed-By: James Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
@eljefedelrodeodeljefe This does not land cleanly in LTS. Added dont-land label. Please feel free to manually backport |
Checklist
Affected core subsystem(s)
doc
Description of change
util.inherits breaks the prototype chain. A fix does not seem
useful, since ES6 extends provides language level support for the
same functionality.
This commit starts phase out mentions of the method.
Fixes: #4179, #6512