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: Alias EventEmitter.protoype.removeListener as EventEmitter.prototype.off #540

Closed
wants to merge 2 commits into from
Closed

events: Alias EventEmitter.protoype.removeListener as EventEmitter.prototype.off #540

wants to merge 2 commits into from

Conversation

yosuke-furukawa
Copy link
Member

EventEmitter should have off alias for io.js beginners, preventing event leaks, API symmetry etc....

I understand the historical reason why events does not have off alias.

nodejs/node-v0.x-archive#3338
nodejs/node-v0.x-archive#5352

Of course, I understand the API of events is frozen in io.js. However io.js have already added the new API getMaxListeners.
And this change does not break backward compatibility. I really would like to add this alias in io.js.

I know io.js follows semantic versioning. If this changes is not patch version (v1.0.x), I can wait for io.js to merge this change until minor version up (v1.1.x).

@quantizor
Copy link

I like this -- it feels like a more natural antonym to the .on() method. Certainly less verbose! 👍

@tellnes
Copy link
Contributor

tellnes commented Jan 22, 2015

+1 for a less verbose name
+1 for consistency with other enviroments
-1 for backwardscompatibility (it will probably break someones code, ref nodejs/node-v0.x-archive#3922)
-1 for making the api more cluttered

@toastynerd
Copy link
Contributor

-1 I don't really thinks it makes sense to 'off' an event.

@ruimarinho
Copy link

+1 for API symmetry/consistency. Several libs implement the on/off pattern, so it's really weird to use on but not off.

@rvagg
Copy link
Member

rvagg commented Jan 23, 2015

it's not really symmetry though is it? the "on" is meant to red like: "for this object, on this event, do this", and "off" doesn't quite fit.

@yosuke-furukawa
Copy link
Member Author

Some famous libraries have on and off. So I feel on 's antonym is off.

jQuery has on and off for aliasing addEventListener, removeEventListener.

other front-end libraries also have on and off. e.g. React-event, Vue.js

Some EventEmitter forks have off alias.

So I feel EventEmitter need to have off method.
I am afraid that some developers think removeListener is not important.

@wavded
Copy link
Contributor

wavded commented Jan 23, 2015

off does feel odd IMO, but whatevs, I've seen un as another shortened alias in libraries too. if clarity is a thing, i've always thought bind and unbind were pretty clear.

@yosuke-furukawa
Copy link
Member Author

I also have seen un. However I have ever seen off so many times. Actually, I also think bind and unbind
were clear. However io.js should not change the existing alias on.

@Fishrock123
Copy link
Contributor

.forget()

@sam-github
Copy link
Contributor

-1. on is a nice alias for addListener, and it gets called a lot. removeListener... not so often. increasing the number of aliases confuses the API. node picked a name, lets stick with it.

And off is weird. event listeners aren't light switches. "on" has a number of meanings, "off" is the opposite of only some of those meanings. "under" is also an antonym of "on"... but neither "under" nor "off" are antonyms of the meaning of "on" used by EE.

@jonathanong
Copy link
Contributor

+0, but i would say this is semver-major. not a big fan of aliases.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 28, 2015

-1. I think it should be semver-minor because it's not a breaking change. Can we set up some sort of vote so that we can either land or close this?

@rvagg
Copy link
Member

rvagg commented Jan 28, 2015

@cjihrig I believe there's enough -1's to just close this

@cjihrig cjihrig closed this Jan 28, 2015
@yosuke-furukawa
Copy link
Member Author

hm, no problem. Thank you for your comments.

@ljharb
Copy link
Member

ljharb commented Nov 4, 2015

This came up again in a very large codebase where we're migrating to an EventEmitter - everything already uses "on" and "off", like most event libs, and we were shocked to find that "on" exists but "off" doesn't. The fact that it's an alias is irrelevant to discoverability or usability imo - if we don't like aliases, great - let's remove "on". If we can't or don't want to remove it (which is fine), then why would it be hard to add a (semver-minor, not major) alias that makes the API more consistent?

@ChALkeR ChALkeR added events Issues and PRs related to the events subsystem / EventEmitter. semver-minor PRs that contain new features and should be released in the next minor version. labels Nov 4, 2015
@ChALkeR
Copy link
Member

ChALkeR commented Nov 4, 2015

+0 from me.

@Fishrock123
Copy link
Contributor

I'm going to re-quote @sam-github from above:

And off is weird. event listeners aren't light switches. "on" has a number of meanings, "off" is the opposite of only some of those meanings. "under" is also an antonym of "on"... but neither "under" nor "off" are antonyms of the meaning of "on" used by EE.

If we are going to add something else I think it should be .forget().

(Even then I'm still -1 though.)

@ljharb
Copy link
Member

ljharb commented Nov 4, 2015

If we have "forget" we'd need the corresponding english antonym "remember" - which wouldn't make any sense, so I don't think that would work.

@ChALkeR
Copy link
Member

ChALkeR commented Nov 5, 2015

@Fishrock123 on/off is rather popular naming scheme for events in various libraries, while on/forget isn't. When people see that there is an on, some suppose that there should be an off.

Refs:

Edit: updated, added eventemitter3 to the list.

@ljharb
Copy link
Member

ljharb commented Nov 5, 2015

I'd be surprised if anyone, lacking knowledge of this particular core module quirk, would expect there not to be an off if there's an on.

@Fishrock123
Copy link
Contributor

Again, see the quote, whether or not there's an on, off doesn't make sense.

@ljharb
Copy link
Member

ljharb commented Nov 5, 2015

@Fishrock123 it doesn't make sense to you, or to @sam-github. It makes perfect sense to many developers. For example, I might see an event emitter, while not as a light switch, as a switch on a walkie talkie - where when "on", i hear things that come in on the channel, but when i want to stop listening, i turn it "off". There's many kinds of on/off switches so I'm sure we can anecdotally come up with a bunch that do, and a bunch that don't, make sense as metaphors for event emitters, but I don't see how that's convincing or valuable in either direction.

@Fishrock123
Copy link
Contributor

Ah, I get it, that makes sense I suppose.

@ChALkeR
Copy link
Member

ChALkeR commented Nov 5, 2015

@Fishrock123 The fact how exactly the method opposite to on should be named to be semantically correct isn't actually relevant here.

cat is an animal, and mount looks like a mountain.

Almost everyone out there is using on/off names for the event systems in js libraries. Check out jQuery, event-emmiter, or EventEmitter2.

The only reason for adding an off would be to be more compatible and obvious here, given the naming scheme that other libraries are using. There are exactly no reasons for adding a forget, because the actual functionality is already present.

@thefourtheye
Copy link
Contributor

on kind of gives me a feel that the expression is readable. For example, when I see something like this, inputStream.on('data', ...), in my mind, I read it like "data event on inputStream" or "on data event". But, I am not able to imagine having off like this. (PS: Not a native English speaker)

@ljharb
Copy link
Member

ljharb commented Nov 5, 2015

Everyone clearly has lots of (valid) colliding subjective opinions on whether "on" or "off" makes sense or not, and on what metaphor makes the most sense for events, et cetera. I don't think we're going to resolve anything to anyone's satisfaction by throwing personal aesthetic preferences back and forth :-)

Personally I think all of the above is irrelevant. Unless we're talking about removing "on" - which would be a conceptually major, semver-major breaking change - we're stuck with it for the forseeable future.

Therefore, the real question is, what should the API look like? Currently there's a nice pairing of addListener/removeListener. on exists as an alias for addListener - but there isn't a companion to "on" in the API - so it's asymmetric. "off" is the word that, objectively based on library usage, pairs best with "on". So, I think it makes sense to add "off" as an alias to removeListener.

@sam-github
Copy link
Contributor

fwiw, I retract my -1. I'm now +/- 0. Maybe soeone would expect a .off() if there is a .on(). But maybe they wouldn't expect to have pass a function to it, and expect it to be the equivalent of removeAllListeners()

@ljharb
Copy link
Member

ljharb commented Mar 15, 2016

Any hope of revisiting this, or at least re-gathering -1/0/+1s from folks?

@ChALkeR
Copy link
Member

ChALkeR commented Jun 17, 2016

@ljharb Actually, I'm leaning towards a +1 on this. It will improve interoperability with all the other event emitter libraries on npm, and on/off is a de-facto standard for that.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 17, 2016

Just a note: this

Of course, I understand the API of events is frozen in io.js.

is not correct anymore. events module is Stable now, not Frozen.

@ljharb
Copy link
Member

ljharb commented Jun 17, 2016

I'd still love to see this added.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 25, 2016

Updated stats at #540 (comment), also added eventemitter3 which also supports .off.

@Delapouite
Copy link
Contributor

Adding this alias would also be handy in Socket.io which inherits from EventEmitter.

@ljharb
Copy link
Member

ljharb commented Apr 22, 2017

Any chance this could get reopened/reconsidered?

@ljharb
Copy link
Member

ljharb commented Apr 12, 2018

Update: #17156 will theoretically land this in node v10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.