Skip to content

Conversation

@fuxingZhang
Copy link
Contributor

@fuxingZhangfuxingZhang commented Feb 18, 2020

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

constEventEmitter=require('events');constmyEmitter=newEventEmitter();constsym=Symbol('symbol');constfn=()=>{};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-botnodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Feb 18, 2020
@addaleaxaddaleax mentioned this pull request Feb 18, 2020
4 tasks
Copy link
Member

@addaleaxaddaleax 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
ContributorAuthor

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

@cjihrigcjihrig 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';constcommon=require('../common');constassert=require('assert');constEventEmitter=require('events');constmyEmitter=newEventEmitter();constsym=Symbol('symbol');constfn=()=>{};myEmitter.on(sym,fn);myEmitter.on('removeListener',common.mustCall((...args)=>{assert.deepStrictEqual(args,[sym,fn]);}));myEmitter.removeAllListeners();

Copy link
Member

@BridgeARBridgeAR 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
ContributorAuthor

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

'use strict';constcommon=require('../common');constassert=require('assert');constEventEmitter=require('events');constmyEmitter=newEventEmitter();constsym=Symbol('symbol');constfn=()=>{};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

@BridgeARBridgeAR 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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Shelley Vohr <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member

Trott commented Mar 7, 2020

Landed in 1b3dbc9.

Thanks for the contribution! 🎉

@TrottTrott 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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Shelley Vohr <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MylesBorinsMylesBorins 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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Shelley Vohr <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Shelley Vohr <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
@codebyterecodebytere 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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Shelley Vohr <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Shelley Vohr <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.eventsIssues and PRs related to the events subsystem / EventEmitter.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

12 participants

@fuxingZhang@nodejs-github-bot@ronag@Trott@jasnell@addaleax@lpinca@codebytere@cjihrig@BridgeAR@ZYSzys@jamesgeorge007