Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix #3425: removeAllListeners should delete array #3431

Closed
wants to merge 1 commit into from

3 participants

Reid Burke Nathan Rajlich isaacs
Reid Burke

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 Burke 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
Nathan Rajlich
Owner

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

Reid Burke

@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 Burke 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 Burke
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)

Nathan Rajlich 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 Burke

    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.
11 lib/events.js
View
@@ -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 Burke
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)

Nathan Rajlich 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;
};
13 test/simple/test-event-emitter-remove-all-listeners.js
View
@@ -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.