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: use missing validators #38893

Closed
wants to merge 4 commits into from
Closed

Conversation

VoltrexKeyva
Copy link
Member

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 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 addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 1, 2021
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

VoltrexKeyva commented Jun 1, 2021

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 😄

@VoltrexKeyva
Copy link
Member Author

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

@VoltrexKeyva VoltrexKeyva deleted the patch-3 branch June 1, 2021 21:59
VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Jun 1, 2021
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 <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
targos pushed a commit that referenced this pull request Jun 11, 2021
PR-URL: #38896
Refs: #38893
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
danielleadams pushed a commit that referenced this pull request Jun 17, 2021
PR-URL: #38896
Refs: #38893
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
richardlau pushed a commit that referenced this pull request Jul 19, 2021
PR-URL: #38896
Refs: #38893
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
richardlau pushed a commit that referenced this pull request Jul 20, 2021
PR-URL: #38896
Refs: #38893
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
PR-URL: nodejs#38896
Refs: nodejs#38893
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
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