fix: correct removeAllListeners in case no event is passed #2088
Conversation
Correct handling of patched removeAllListeners() in AbstractAsyncHooksContextManager in case it's called without argument. In this case all listener types should be removed. Additionally it's imporant to forward arguments instead of passing undefined as node.js checks arguments.length.
Flarna
requested review from
dyladan,
johnbley,
legendecas,
markwolff,
mayurkale22,
mwear,
naseemkullah,
obecny,
OlivierAlbertini and
vmarchaud
as code owners
Codecov Report
@@ Coverage Diff @@
## main #2088 +/- ##
==========================================
+ Coverage 92.53% 92.55% +0.02%
==========================================
Files 133 133
Lines 4848 4851 +3
Branches 1000 1002 +2
==========================================
+ Hits 4486 4490 +4
+ Misses 362 361 -1
|
| @@ -146,10 +146,14 @@ export abstract class AbstractAsyncHooksContextManager | |||
| const contextManager = this; | |||
| return function (this: never, event: string) { | |||
| const map = contextManager._getPatchMap(ee); | |||
| if (map?.[event] !== undefined) { | |||
| delete map[event]; | |||
| if (map !== undefined) { | |||
There was a problem hiding this comment.
const map = null;
map !== undefined;
map['aa'] !== undefined;
// error
| if (map !== undefined) { | |
| if (map) { |
There was a problem hiding this comment.
I used !== undefined here because it's used at a lot places in this file. Usually I prefer != null.
There was a problem hiding this comment.
but if the map is null then the condition will pass and you will have undefined object exception
There was a problem hiding this comment.
!= null would catch both null and undefined
There was a problem hiding this comment.
Also, the map will never be null in this case, only undefined. If a dev made a change which allowed this to be null, the test would fail.
There was a problem hiding this comment.
I agree that map is never null here, i don't see the benefit of checking for both undefined and null
There was a problem hiding this comment.
As said I usually use != null. But there are a lot places in this file where !== undefined is already used so the no 1 rule 'When in Rome, Do as the Romans Do' wins.
I would prefer to do cleanups like this in a non bugfix PR.
There was a problem hiding this comment.
well if (map) { would simply do the trick and will be a bullet proof for any future changes, you don't need to use != null
| @@ -146,10 +146,14 @@ export abstract class AbstractAsyncHooksContextManager | |||
| const contextManager = this; | |||
| return function (this: never, event: string) { | |||
| const map = contextManager._getPatchMap(ee); | |||
| if (map?.[event] !== undefined) { | |||
| delete map[event]; | |||
| if (map !== undefined) { | |||
There was a problem hiding this comment.
well if (map) { would simply do the trick and will be a bullet proof for any future changes, you don't need to use != null
Which problem is this PR solving?
fixes #2068
Short description of the changes
Correct handling of patched
removeAllListeners()inAbstractAsyncHooksContextManagerin case it's called without argument.In this case all listener types should be removed.
Additionally it's important to forward arguments instead of passing
undefinedas node.js checksarguments.length.