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: prevent undefined as an event name #21007

Closed
wants to merge 1 commit into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented May 28, 2018

This commit prevents undefined as an event name in addListener(), removeListener(), and emit().

Refs: #20923

A few things to note:

  • The motivation here is that undefined as an event name is almost certainly a mistake. I considered giving null the same treatment, but decided not to as a first draft.
  • The validation message claims that the 'eventName' input must be a string or symbol. Our documentation calls the parameter 'eventName', even though the code doesn't. The docs also call for a string or symbol, which simply isn't true.
  • I tried to make this a minimally breaking change. If we require the input to be strictly a string or symbol, it will probably break a lot of perfectly good code that uses things like numbers. This kind of breakage seems unnecessary.
  • I'm currently validating the addition and removal of listeners, as well as emit(). I considered adding validation to the methods that retrieve listeners, but I'm not sure that's necessary since they are only querying state, not modifying it.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

This commit prevents undefined as an event name in
addListener(), removeListener(), and emit().
@cjihrig cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 28, 2018
@nodejs-github-bot nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label May 28, 2018
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.

Big 💙 for keeping this change in behaviour minimal. :)

@jdalton
Copy link
Member

jdalton commented May 28, 2018

If according to the message:

The "eventName" argument must be one of type string or symbol.

Why not restrict to that first thing in the affected methods instead of keying in on undefined specifically (some back compat concern)?

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

It would be good to also validate null and objects. Both of them will clearly not behave as expected.

function validateEventName(name, paramName) {
if (name === undefined) {
const { ERR_INVALID_ARG_TYPE } = lazyErrors();
const err = new ERR_INVALID_ARG_TYPE(paramName, ['string', 'symbol'], name);
Copy link
Member

Choose a reason for hiding this comment

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

are there other possible paramNames? Otherwise I think it would be best to inline that string.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree.

@mscdex
Copy link
Contributor

mscdex commented May 28, 2018

removeAllListeners() needs to be modified as well in the right places.

Copy link
Member

@jdalton jdalton left a comment

Choose a reason for hiding this comment

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

If the guard is to ensure the "eventName" argument must be one of type string or symbol then it should be applied to more than just undefined. Blocking just because there are a couple approvals and I think it needs a bit more work.

@cjihrig cjihrig closed this May 29, 2018
@addaleax
Copy link
Member

@cjihrig I’m sorry you closed this. Would you mind if I picked it up again? I think this is strictly an improvement, so I’d like to see it make its way into Node and blocking it seems inappropriate to me.

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. 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.

9 participants