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

events: refactor to use validator #45448

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Lxxyx
Copy link
Member

@Lxxyx Lxxyx commented Nov 13, 2022

Use validateNumber() for events(setMaxListeners/defaultMaxListeners).

If passed wrong parameter types, validateNumber() will throw ERR_INVALID_ARG_TYPE instead of original error code: ERR_OUT_OF_RANGE. I think ERR_INVALID_ARG_TYPE is more accurate.

@nodejs-github-bot nodejs-github-bot added events Issues and PRs related to the events subsystem / EventEmitter. needs-ci PRs that need a full CI run. labels Nov 13, 2022
@Trott Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 13, 2022
@Trott
Copy link
Member

Trott commented Nov 13, 2022

At least in theory, something that changes an error code is semver major so I added that label. But if others feel strongly that it isn't, I'm ok with it being removed and treating this as a bug fix.

@anonrig
Copy link
Member

anonrig commented Nov 13, 2022

I agree with @Trott. There might be some packages that depend on this error message, and therefore I'm good with semver-major label.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Let's not validate the error messages.

test/parallel/test-event-emitter-max-listeners.js Outdated Show resolved Hide resolved
test/parallel/test-event-emitter-max-listeners.js Outdated Show resolved Hide resolved
test/parallel/test-event-emitter-max-listeners.js Outdated Show resolved Hide resolved
test/parallel/test-event-emitter-max-listeners.js Outdated Show resolved Hide resolved
test/parallel/test-event-emitter-max-listeners.js Outdated Show resolved Hide resolved
test/parallel/test-event-emitter-max-listeners.js Outdated Show resolved Hide resolved
test/parallel/test-event-emitter-max-listeners.js Outdated Show resolved Hide resolved
test/parallel/test-event-emitter-max-listeners.js Outdated Show resolved Hide resolved
Trott added a commit to Trott/io.js that referenced this pull request Nov 14, 2022
As much as I would like to do this everywhere and then modify the lint
rule to enforce it, the churn would be too big. However if we're going
to have relatively frequent nits for this sort of thing (as we do), I'd
prefer we migrate a few files at a time to never actually getting around
to doing it.

Ref: nodejs#45448 (review)
Trott added a commit to Trott/io.js that referenced this pull request Nov 15, 2022
As much as I would like to do this everywhere and then modify the lint
rule to enforce it, the churn would be too big. However if we're going
to have relatively frequent nits for this sort of thing (as we do), I'd
prefer we migrate a few files at a time to never actually getting around
to doing it.

Ref: nodejs#45448 (review)
aduh95 pushed a commit to aduh95/node that referenced this pull request Nov 15, 2022
As much as I would like to do this everywhere and then modify the lint
rule to enforce it, the churn would be too big. However if we're going
to have relatively frequent nits for this sort of thing (as we do), I'd
prefer we migrate a few files at a time to never actually getting around
to doing it.

Ref: nodejs#45448 (review)
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM as a semver major. This should have a benchmark run before it lands.

nodejs-github-bot pushed a commit that referenced this pull request Nov 22, 2022
As much as I would like to do this everywhere and then modify the lint
rule to enforce it, the churn would be too big. However if we're going
to have relatively frequent nits for this sort of thing (as we do), I'd
prefer we migrate a few files at a time to never actually getting around
to doing it.

Ref: #45448 (review)
PR-URL: #45466
Reviewed-By: James M Snell <jasnell@gmail.com>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Nov 23, 2022
As much as I would like to do this everywhere and then modify the lint
rule to enforce it, the churn would be too big. However if we're going
to have relatively frequent nits for this sort of thing (as we do), I'd
prefer we migrate a few files at a time to never actually getting around
to doing it.

Ref: nodejs#45448 (review)
PR-URL: nodejs#45466
Reviewed-By: James M Snell <jasnell@gmail.com>
ruyadorno pushed a commit that referenced this pull request Nov 24, 2022
As much as I would like to do this everywhere and then modify the lint
rule to enforce it, the churn would be too big. However if we're going
to have relatively frequent nits for this sort of thing (as we do), I'd
prefer we migrate a few files at a time to never actually getting around
to doing it.

Ref: #45448 (review)
PR-URL: #45466
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
As much as I would like to do this everywhere and then modify the lint
rule to enforce it, the churn would be too big. However if we're going
to have relatively frequent nits for this sort of thing (as we do), I'd
prefer we migrate a few files at a time to never actually getting around
to doing it.

Ref: #45448 (review)
PR-URL: #45466
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
As much as I would like to do this everywhere and then modify the lint
rule to enforce it, the churn would be too big. However if we're going
to have relatively frequent nits for this sort of thing (as we do), I'd
prefer we migrate a few files at a time to never actually getting around
to doing it.

Ref: #45448 (review)
PR-URL: #45466
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
As much as I would like to do this everywhere and then modify the lint
rule to enforce it, the churn would be too big. However if we're going
to have relatively frequent nits for this sort of thing (as we do), I'd
prefer we migrate a few files at a time to never actually getting around
to doing it.

Ref: #45448 (review)
PR-URL: #45466
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
As much as I would like to do this everywhere and then modify the lint
rule to enforce it, the churn would be too big. However if we're going
to have relatively frequent nits for this sort of thing (as we do), I'd
prefer we migrate a few files at a time to never actually getting around
to doing it.

Ref: #45448 (review)
PR-URL: #45466
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 5, 2023
As much as I would like to do this everywhere and then modify the lint
rule to enforce it, the churn would be too big. However if we're going
to have relatively frequent nits for this sort of thing (as we do), I'd
prefer we migrate a few files at a time to never actually getting around
to doing it.

Ref: #45448 (review)
PR-URL: #45466
Reviewed-By: James M Snell <jasnell@gmail.com>
@aduh95
Copy link
Contributor

aduh95 commented Sep 29, 2023

@Lxxyx this needs a rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants