Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix #3425: removeAllListeners should delete array #3431

Closed
wants to merge 1 commit into from

3 participants

@reid

When removeAllListeners is called, the listeners array is deleted to maintain compatibility with v0.6.

This request fixes issue #3425 by reverting 78dc13f. In addition to the revert, I've updated the tests to match the reverted behavior.

@reid reid Fix #3425: removeAllListeners should delete array
When removeAllListeners is called, the listeners array
is deleted to maintain compatibility with v0.6.

Reverts "events: don't delete the listeners array"

This reverts commit 78dc13f.

Conflicts:

	test/simple/test-event-emitter-remove-all-listeners.js
2fd00b8
@TooTallNate
Owner

See #3425 (comment) for my alternate proposal for a solution.

@reid

@isaacs This needs an API doc clarification, but I haven't yet come up with something that doesn't refer to EventEmitter's internals.

@reid reid commented on the diff
lib/events.js
@@ -228,15 +228,8 @@ EventEmitter.prototype.removeAllListeners = function(type) {
return this;
}
- var events = this._events && this._events[type];
- if (!events) return this;
-
- if (isArray(events)) {
- events.splice(0);
- } else {
- this._events[type] = null;
- }
-
+ // does not use listeners(), so no side effect of creating _events[type]
@reid
reid added a note

@TooTallNate The reverted behavior explicitly tries to avoid calling listeners() to avoid creating _events[type]. Is that still needed?. Regarding your proposal in #3425 (comment)

@TooTallNate Owner

I personally think that's overkill. I don't consider removeAllListeners() to be a hot code path enough to justify that, but others would need to weigh in on that as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@isaacs
Owner

Landed, along with an alternate fix for the thing that this was fixing, and clarification in the docs. Thanks, everyone :)

@isaacs isaacs closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 13, 2012
  1. @reid

    Fix #3425: removeAllListeners should delete array

    reid authored
    When removeAllListeners is called, the listeners array
    is deleted to maintain compatibility with v0.6.
    
    Reverts "events: don't delete the listeners array"
    
    This reverts commit 78dc13f.
    
    Conflicts:
    
    	test/simple/test-event-emitter-remove-all-listeners.js
This page is out of date. Refresh to see the latest.
View
11 lib/events.js
@@ -228,15 +228,8 @@ EventEmitter.prototype.removeAllListeners = function(type) {
return this;
}
- var events = this._events && this._events[type];
- if (!events) return this;
-
- if (isArray(events)) {
- events.splice(0);
- } else {
- this._events[type] = null;
- }
-
+ // does not use listeners(), so no side effect of creating _events[type]
@reid
reid added a note

@TooTallNate The reverted behavior explicitly tries to avoid calling listeners() to avoid creating _events[type]. Is that still needed?. Regarding your proposal in #3425 (comment)

@TooTallNate Owner

I personally think that's overkill. I don't consider removeAllListeners() to be a hot code path enough to justify that, but others would need to weigh in on that as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ if (type && this._events && this._events[type]) this._events[type] = null;
return this;
};
View
13 test/simple/test-event-emitter-remove-all-listeners.js
@@ -39,10 +39,15 @@ e1.removeAllListeners('baz');
assert.deepEqual(e1.listeners('foo'), [listener]);
assert.deepEqual(e1.listeners('bar'), []);
assert.deepEqual(e1.listeners('baz'), []);
-// identity check, the array should not change
-assert.equal(e1.listeners('foo'), fooListeners);
-assert.equal(e1.listeners('bar'), barListeners);
-assert.equal(e1.listeners('baz'), bazListeners);
+// after calling removeAllListeners,
+// the old listeners array should stay unchanged
+assert.deepEqual(fooListeners, [listener]);
+assert.deepEqual(barListeners, [listener]);
+assert.deepEqual(bazListeners, [listener, listener]);
+// after calling removeAllListeners,
+// new listeners arrays are different from the old
+assert.notEqual(e1.listeners('bar'), barListeners);
+assert.notEqual(e1.listeners('baz'), bazListeners);
var e2 = new events.EventEmitter();
e2.on('foo', listener);
Something went wrong with that request. Please try again.