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

process: replace validator #41660

Closed

Conversation

VoltrexKeyva
Copy link
Member

Replace the validateFunction() validator with the
validateCallback() validator to validate callbacks and keep
consistency.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Jan 23, 2022
Replace the `validateFunction()` validator with the
`validateCallback()` validator to validate callbacks and keep
consistency.
@Trott Trott added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Jan 23, 2022
@Trott
Copy link
Member

Trott commented Jan 23, 2022

Changing an error code is usually semver-major but I could the case for treating this as a bug fix. @nodejs/tsc Thoughts?

@Trott
Copy link
Member

Trott commented Jan 23, 2022

Personally, I'd like to see us reduce the specificity of error codes in general. I think ERR_INVALID_ARG_TYPE is fine and we can get rid of ERR_INVALID_CALLBACK entirely. But that would definitely be semver-major and outside the scope of this PR.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

This has the potential to break a lot of users' tests when they use queueMicrotask because of the error code change doesn't it?

Intuitively, this sort of breakage isn't worth it in a stable API - it's not a lot of breakage but it's not like we feel strongly the error should change.

(I also agree with @Trott the fact we have a lot of error codes does not necessarily help our users)

@aduh95
Copy link
Contributor

aduh95 commented Jan 23, 2022

+1 to label this as semver-major, changing an error code is a potential breaking change.

+1 to get rid of ERR_INVALID_CALLBACK in favor of ERR_INVALID_ARG_TYPE at some point. validateCallback is not even checking if the parameter is actually a callback, if you pass a class it won't throw:

validateCallback(class{}); // doesn't throw

@RaisinTen
Copy link
Contributor

Changing an error code is usually semver-major but I could the case for treating this as a bug fix.

Bug fixes can be server-major too, so I'm adding the label back.

@RaisinTen RaisinTen added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 23, 2022
@VoltrexKeyva
Copy link
Member Author

VoltrexKeyva commented Jan 23, 2022

I would be more than happy to remove the ERR_INVALID_CALLBACK error entirely and replace it with ERR_INVALID_ARG_TYPE, but wouldn't that still cause error code changes? Since some APIs still seem to be using/throwing the ERR_INVALID_CALLBACK error such as the process.nextTick() method.

validateCallback is not even checking if the parameter is actually a callback, if you pass a class it won't throw:

validateCallback(class{}); // doesn't throw

That's the exact same case with the validateFunction() validator since both of the validateFunction() and validateCallback() validators have the exact same check, only difference being that they throw a different error.

We could entirely remove the validateCallback() validator and the ERR_INVALID_CALLBACK error and replace it with the validateFunction() validator, but we should change the behavior of the validator for it to be able to a differentiate a class and a function, so it would also throw on classes.

@Trott
Copy link
Member

Trott commented Jan 23, 2022

would be more than happy to remove the ERR_INVALID_CALLBACK error entirely and replace it with ERR_INVALID_ARG_TYPE, but wouldn't that still cause error code changes?

Yes, that would be a semver-major change (but one that is worth making in my opinion, especially if we can remove a few other codes in the same semver-major bump).

VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Jan 24, 2022
VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Jan 24, 2022
VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Jan 24, 2022
@VoltrexKeyva
Copy link
Member Author

#41678

@VoltrexKeyva VoltrexKeyva deleted the process-replace-validator branch January 24, 2022 16:21
VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Jan 24, 2022
VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Jan 24, 2022
VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Jan 24, 2022
VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Jan 24, 2022
VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Jan 24, 2022
VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Jan 24, 2022
VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Jan 24, 2022
VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Jan 24, 2022
VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Jan 25, 2022
VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Jan 25, 2022
VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Jan 26, 2022
VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Jan 26, 2022
VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Feb 1, 2022
VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Feb 2, 2022
VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Feb 2, 2022
VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request Feb 3, 2022
jasnell pushed a commit that referenced this pull request Feb 5, 2022
Refs: #41660

PR-URL: #41678
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@RaisinTen RaisinTen mentioned this pull request Apr 20, 2022
4 tasks
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. process Issues and PRs related to the process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants