FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

util: use missing validators by VoltrexKeyva · Pull Request #38893 · nodejs/node · GitHub

/ node Public

util: use missing validators - #38893

Closed
VoltrexKeyva wants to merge 4 commits into
nodejs:masterfrom
VoltrexKeyva:patch-3
Closed

util: use missing validators#38893
VoltrexKeyva wants to merge 4 commits into
nodejs:masterfrom
VoltrexKeyva:patch-3

Conversation

Copy link
Copy Markdown
Contributor

The inherits() method in the util lib module is not using validators which others do use.

The `inherits()` method in the `util` lib module is not using validators which others do use.
github-actions Bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Jun 1, 2021
Removed the `ERR_INVALID_ARG_TYPE` constructor which was only used in the `inherits()` method which is replaced now.
The value must be a function to test the type of the prototype since validators first check if the value is a function.
The message expects to fail with the value `undefined`, not `null`.
addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 1, 2021

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is a breaking change, because it makes the requirements on ctor and superCtor a bit stricter.

Since the docs already mention that one should use ES6 class extends instead, can we maybe just mark util.inherits with the new official "legacy" status instead?

VoltrexKeyva commented Jun 1, 2021
edited
Loading

Copy link
Copy Markdown
Contributor Author

This is a breaking change, because it makes the requirements on ctor and superCtor a bit stricter.

Since the docs already mention that one should use ES6 class extends instead, can we maybe just mark util.inherits with the new official "legacy" status instead?

I'm +1 on that, I would like to hear other collaborators/members opinions and see they agree as well 😄

Copy link
Copy Markdown
Contributor Author

Just gonna open another PR referencing this to change the status to legacy, closing.

VoltrexKeyva deleted the patch-3 branch June 1, 2021 21:59
aduh95 pushed a commit to VoltrexKeyva/node that referenced this pull request Jun 9, 2021
PR-URL: nodejs#38896
Refs: nodejs#38893
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
targos pushed a commit that referenced this pull request Jun 11, 2021
PR-URL: #38896
Refs: #38893
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
danielleadams pushed a commit that referenced this pull request Jun 17, 2021
PR-URL: #38896
Refs: #38893
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
richardlau pushed a commit that referenced this pull request Jul 19, 2021
PR-URL: #38896
Refs: #38893
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
richardlau pushed a commit that referenced this pull request Jul 20, 2021
PR-URL: #38896
Refs: #38893
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version. util Issues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL