Skip to content

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