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: fix not emit removeListener #31847

Closed
wants to merge 2 commits into from
Closed

events: fix not emit removeListener #31847

wants to merge 2 commits into from

Conversation

fuxingZhang
Copy link
Contributor

@fuxingZhang fuxingZhang commented Feb 18, 2020

Fix not emit removeListener when eventName type is 'symbol'.

const EventEmitter = require('events');
const myEmitter = new EventEmitter();
const sym = Symbol('symbol');
const fn = () => { };
myEmitter.on(sym, fn);

myEmitter.on('removeListener', (...args) => {
  console.log('removeListener');
  console.log(args, args[0] === sym, args[1] === fn);
});

myEmitter.removeAllListeners()
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Fix not emit removeListener when eventName type is 'symbol'.

```js
const EventEmitter = require('events');
const myEmitter = new EventEmitter();
const sym = Symbol('symbol');
const fn = () => { };
myEmitter.on(sym, fn);

myEmitter.on('removeListener', (...args) => {
  console.log('removeListener');
  console.log(args, args[0] === sym, args[1] === fn);
});

myEmitter.removeAllListeners()
```
@nodejs-github-bot nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Feb 18, 2020
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.

Looks good!

Do you think you could add test for this?

@fuxingZhang
Copy link
Contributor Author

Looks good!

Do you think you could add test for this?

I don't know yet, I'm not sure, but I can learn how to do it tomorrow or the day after tomorrow

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM, and based on your OP, you can probably use something like this as the test:

'use strict';
const common = require('../common');
const assert = require('assert');
const EventEmitter = require('events');
const myEmitter = new EventEmitter();
const sym = Symbol('symbol');
const fn = () => {};
myEmitter.on(sym, fn);

myEmitter.on('removeListener', common.mustCall((...args) => {
  assert.deepStrictEqual(args, [sym, fn]);
}));

myEmitter.removeAllListeners();

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.

LGTM with a test added

When the listener's eventName type is 'symbol' and removeListener is called with no parameters, removeListener should be emitted.
@fuxingZhang
Copy link
Contributor Author

LGTM, and based on your OP, you can probably use something like this as the test:

'use strict';
const common = require('../common');
const assert = require('assert');
const EventEmitter = require('events');
const myEmitter = new EventEmitter();
const sym = Symbol('symbol');
const fn = () => {};
myEmitter.on(sym, fn);

myEmitter.on('removeListener', common.mustCall((...args) => {
  assert.deepStrictEqual(args, [sym, fn]);
}));

myEmitter.removeAllListeners();

Thank you, it was very helpful for me.
I have added the test code to an existing test file ‘test-event-emitter-remove-all-listeners.js’.

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 19, 2020
@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
Member

ronag commented Mar 7, 2020

CI was 404, started another one

Trott pushed a commit that referenced this pull request Mar 7, 2020
Fix removeListener when eventName type is 'symbol'.

```js
const EventEmitter = require('events');
const myEmitter = new EventEmitter();
const sym = Symbol('symbol');
const fn = () => { };
myEmitter.on(sym, fn);

myEmitter.on('removeListener', (...args) => {
  console.log('removeListener');
  console.log(args, args[0] === sym, args[1] === fn);
});

myEmitter.removeAllListeners()
```

When the listener's eventName type is 'symbol' and removeListener is
called with no parameters, removeListener should be emitted.

PR-URL: #31847
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member

Trott commented Mar 7, 2020

Landed in 1b3dbc9.

Thanks for the contribution! 🎉

@Trott Trott closed this Mar 7, 2020
MylesBorins pushed a commit that referenced this pull request Mar 9, 2020
Fix removeListener when eventName type is 'symbol'.

```js
const EventEmitter = require('events');
const myEmitter = new EventEmitter();
const sym = Symbol('symbol');
const fn = () => { };
myEmitter.on(sym, fn);

myEmitter.on('removeListener', (...args) => {
  console.log('removeListener');
  console.log(args, args[0] === sym, args[1] === fn);
});

myEmitter.removeAllListeners()
```

When the listener's eventName type is 'symbol' and removeListener is
called with no parameters, removeListener should be emitted.

PR-URL: #31847
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 10, 2020
codebytere pushed a commit that referenced this pull request Mar 16, 2020
Fix removeListener when eventName type is 'symbol'.

```js
const EventEmitter = require('events');
const myEmitter = new EventEmitter();
const sym = Symbol('symbol');
const fn = () => { };
myEmitter.on(sym, fn);

myEmitter.on('removeListener', (...args) => {
  console.log('removeListener');
  console.log(args, args[0] === sym, args[1] === fn);
});

myEmitter.removeAllListeners()
```

When the listener's eventName type is 'symbol' and removeListener is
called with no parameters, removeListener should be emitted.

PR-URL: #31847
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
Fix removeListener when eventName type is 'symbol'.

```js
const EventEmitter = require('events');
const myEmitter = new EventEmitter();
const sym = Symbol('symbol');
const fn = () => { };
myEmitter.on(sym, fn);

myEmitter.on('removeListener', (...args) => {
  console.log('removeListener');
  console.log(args, args[0] === sym, args[1] === fn);
});

myEmitter.removeAllListeners()
```

When the listener's eventName type is 'symbol' and removeListener is
called with no parameters, removeListener should be emitted.

PR-URL: #31847
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
codebytere pushed a commit that referenced this pull request Mar 23, 2020
Fix removeListener when eventName type is 'symbol'.

```js
const EventEmitter = require('events');
const myEmitter = new EventEmitter();
const sym = Symbol('symbol');
const fn = () => { };
myEmitter.on(sym, fn);

myEmitter.on('removeListener', (...args) => {
  console.log('removeListener');
  console.log(args, args[0] === sym, args[1] === fn);
});

myEmitter.removeAllListeners()
```

When the listener's eventName type is 'symbol' and removeListener is
called with no parameters, removeListener should be emitted.

PR-URL: #31847
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 30, 2020
Fix removeListener when eventName type is 'symbol'.

```js
const EventEmitter = require('events');
const myEmitter = new EventEmitter();
const sym = Symbol('symbol');
const fn = () => { };
myEmitter.on(sym, fn);

myEmitter.on('removeListener', (...args) => {
  console.log('removeListener');
  console.log(args, args[0] === sym, args[1] === fn);
});

myEmitter.removeAllListeners()
```

When the listener's eventName type is 'symbol' and removeListener is
called with no parameters, removeListener should be emitted.

PR-URL: #31847
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. events Issues and PRs related to the events subsystem / EventEmitter.
Projects
None yet
Development

Successfully merging this pull request may close these issues.