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: add off alias to removeListener #17156

Closed
wants to merge 1 commit into
base: master
from

Conversation

@Ulmanb
Contributor

Ulmanb commented Nov 20, 2017

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

Hi, fixes #17102 - this is my first issue #goodnessSquad

@benjamingr

Changes LGTM - let's see if we have consensus on adding it.

@benjamingr

This comment has been minimized.

Member

benjamingr commented Nov 20, 2017

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Nov 20, 2017

One note - even if we do get consensus on adding this feature, I think we should give @yosuke-furukawa the option to update the original PR in #540 (referenced in #17102) if they want to do so.

@apapirovski

This will always be desired by anyone coming to Node having done any sort of front-end work... I don't see much harm to adding it.

@ljharb

ljharb approved these changes Nov 20, 2017

Agree on preferring the original PR if possible, psyched to see this happen!

@jasnell

This comment has been minimized.

Member

jasnell commented Nov 20, 2017

Please make sure to CITGM this.

@addaleax

This comment has been minimized.

Member

addaleax commented Nov 21, 2017

CI & CITGM seem good.

I have to admit I’m surprised by the enthusiasm for this option expressed in the linked issue – this seems like a mediocre name at best, it’s not like you could say “do X off an occasion” in English…

@ljharb

This comment has been minimized.

ljharb commented Nov 21, 2017

The use in a sentence isn’t the attraction; it’s more that the obvious answer to the question “how do i undo ‘on’” is “use ‘off’”. This has been a pain point in node for many years as a result; one that has always taken a line of code to address.

Looking forward to this landing.

@tniessen

This comment has been minimized.

Member

tniessen commented Nov 21, 2017

This has been discussed a lot and there still seem to be a lot of different opinions. I would love to have a very simple vote on this, not with the intention of deriving a decision from it, but because I think that GitHub approvals don't necessarily represent opinions, and I would love to get more feedback on this idea.

This is not limited to organization members, but I would be happy about as many votes from @nodejs/collaborators as possible!


Before voting, please read #17102 for a summary of the most relevant aspects of this change. Feel free to comment your own point of view or any reasons to or not to create .off as an alias. Please consider that the points presented in the issue might represent the author's opinion and are not necessarily objective. Apart from the linked issue, there were previous discussions, namely

One aspect which only occurred to me recently is that .on might have originated from onclick="" attributes and the like, while newer browsers use addEventListener etc.


Please pick at most one of
👍 - I like the idea of having .off as an alias for removeListener within Node.js core despite knowing contrary arguments.
👎 - I do not like the idea of having .off as an alias for removeListener.

And any combination of
😄 - .off should be an alias for removeListener because other frameworks have it.
🎉 - I think off is a good name for a function which removes an event listener.
😕 - I don't like using the word off to remove event listeners, e.g. because it does not sound natural.
❤ - I prefer an API without unnecessary aliases, e.g. for functions which are called much less frequently than others, or because this might conflict with classes inheriting from EventEmitter.

(It seems like the heart emoji is not always visible in Firefox, but you should still be able to select it from the list when voting. The emoji is labelled "Heart" when you hover over the empty space.)

If you vote 👍 or 👎 for reasons other than those outlined in #17102, feel free to leave a comment to tell us about your reasons!

@devsnek

This comment has been minimized.

Member

devsnek commented Nov 21, 2017

I've always felt like on and removeListener were from two different libraries. ideally I think it should have been addListener and removeListener. perhaps both aliases could be added, although that might get a bit confusing. I just see it as a consistency thing, where someone shouldnt have to go to the docs because you can't guess one if you only know the other.

@apapirovski

This comment has been minimized.

Member

apapirovski commented Nov 21, 2017

@ljharb

This comment has been minimized.

ljharb commented Nov 21, 2017

@devsnek this would be a different discussion if both aliases were being added; but “on” and “addEventListener” have always been aliases - adding “off” completes the set.

@gireeshpunathil

This comment has been minimized.

Member

gireeshpunathil commented Nov 21, 2017

as node is a beautiful abstraction of low level events and code around those events, this is a wonderful construct that aligns to this philosophy.

@mcollina

I’m extremely -1 to add a 3 char method to the base of a good chunk of the ecosystem, mainly because of possible conflicts. Unfortunately, I’m away for the next two weeks to better formulate my thoughts.

I’m marking it as “request changes” mainly to make my opinion visible.

@sam-github

This comment has been minimized.

Member

sam-github commented Nov 21, 2017

Besides the unnecessary clutter, off() doesn't even achieve the goal of making the API easier to use without looking at the docs, because off() isn't the opposite of on(), not in the sense that on() is used in this API.

@apapirovski

This comment has been minimized.

Member

apapirovski commented Nov 21, 2017

Besides the unnecessary clutter, off() doesn't even achieve the goal of making the API easier to use without looking at the docs, because off() isn't the opposite of on(), not in the sense that on() is used in this API.

Of course it makes it easier. Anyone coming from any front-end work — which Node.js has become a huge part of — will expect off to be the opposite of on. That's just a fact, it's pointless to argue with it by citing semantics or language or whatever.

I feel like there's just this fundamental clash between developers coming from a different context than the one that had traditionally dominated Node.js usage.

Front-end in general is possibly the largest consumer of Node.js so to ignore that perspective is a big mistake.

The general unwillingness to try to empathize with a large swath of the user-base which is coming at this with a wholly different context is kind of a major turn-off in these debates.

@hashseed

This comment has been minimized.

Member

hashseed commented Nov 21, 2017

I'm curious where else off is used as opposite of on, since front-end is being cited.

@addaleax

This comment has been minimized.

Member

addaleax commented Nov 21, 2017

@hashseed The linked Fixes: issue #17102 has a list :)

@sam-github

This comment has been minimized.

Member

sam-github commented Nov 21, 2017

I'm puzzled by your argument, and your statement that anybody disagreeing with you lacks empathy, perhaps we can focus on the EventEmitter, not personal accusations? If we don't discuss what APIs make sense based on the actual meaning of English words, what do we discuss?

What is it about front-end developers that make them particularly need this when doing back-end development? AFAIK, there is no EventEmitter in the browser.

@apapirovski

This comment has been minimized.

Member

apapirovski commented Nov 21, 2017

What is it about front-end developers that make them particularly need this when doing back-end development? AFAIK, there is no EventEmitter in the browser.

Front-end tooling is currently (very likely) the largest consumer of Node.js... this isn't back-end development.

If we don't discuss what APIs make sense based on the actual meaning of English words, what do we discuss?

Bringing it back to the language is refusing to empathize with where other users are coming from.

I don't have particularly strong feelings on adding off. I have strong feelings on a large swath of the user-base not being properly listened to.

@ianstormtaylor

This comment has been minimized.

ianstormtaylor commented Nov 21, 2017

@tniessen would you consider editing your poll comment to slightly call out #17102 more? I realize that it obviously contains some of my opinions, and would want people to make formulate their own opinion, but I'm also pretty sure that many people are going to just come to this issue, quickly skim it, and start providing their quick opinions without considering the ecosystem as a whole, or any of the past arguments. (As it seems like the comments are already doing.)

@hashseed

This comment has been minimized.

Member

hashseed commented Nov 21, 2017

If on and addListener exist side by side, I would figure for the sake of symmetry that off also should exist.

Or maybe no as it is the reverse of on :D

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Dec 13, 2017

I abstain for now.

@jasnell, I will try to look this up, but not sure how exactly could that be done.
I expect a ton of .off usage, as (~all of the) other events implementations use that, and it would be hard to separate those libs from our implementation and to trace the code that would be potentially broken by this change.

Update: there is a lot of .off = in the codebase, but it's hard to tell which part of those sets .off on EventEmitter objects, as even core API inherits that, so filtering for files with require('events') will be too strict.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Dec 13, 2017

Everyone, do you think if an attempt to override it should warn users? Such overriding would mean one of two things:

  1. It's being overriden with an exact same alias. This case is fixeable in userland with a simple check in the lib that tries to set the alias.
  2. It's being overriden with something else — e.g. a variable or a method which does other things. This case means that something is either broken or is probable to be broken — e.g. (in variable case) if userland code later checks for the .off variable and treats it as a flag, or (in different method case) if some userland code (e.g. different libs) expect different behaviour from .off — Node.js one and overriden one.
@ljharb

This comment has been minimized.

ljharb commented Dec 13, 2017

@ChALkeR I think a warning for overriding it with the same removeEventListener would be kind of silly; but I would definitely think a warning for overriding it with something else would make sense.

However, I don't think either is necessary if it's landing in a semver-major; if we were landing it in a minor (which we're not, based on the current votes in this thread) then I'd hope the second warning you indicated would indeed exist.

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Dec 13, 2017

@MylesBorins for me it was not about timing but about having a conclusion. I am aware that this is not time sensitive.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Dec 14, 2017

@ljharb Well, the warning could be trivially disabled for overriding with .removeEventListener itself, but cases like function(...args) { return this.removeEventListener(...args) } would be harder to detect and I don't think its worth detecting them. So, IMO it should be enabled for everything !== .removeEventListener itself.

Also, this is clearly a semver-major because of breakage potential, and I think it would still make sense to enable such warning even in a semver-major change, because otherwise it would be hard to notice problems — people don't generally re-read all the code searching for what changed, especially if the affected code is hidden deep inside dependencies which are not very actively supported.

@joyeecheung

This comment has been minimized.

Member

joyeecheung commented Dec 14, 2017

I am going to abstain, there is already a majority anyway though.

@danbev

This comment has been minimized.

Member

danbev commented Dec 14, 2017

+1 from me.

@benjamingr

This comment has been minimized.

Member

benjamingr commented Dec 17, 2017

It looks like the TSC has voted in favor of this PR.

CI and CITGM already passed - going to land this tomorrow unless anyone has more questions or comments (or someone will beat me to it) - and @Ulmanb has offered to do a follow-up PR about overriding behavior.

@BridgeAR BridgeAR added author ready and removed tsc-review labels Dec 17, 2017

@benjamingr

This comment has been minimized.

Member

benjamingr commented Dec 20, 2017

Landed in 3bb6f07 🎉

@benjamingr benjamingr closed this Dec 20, 2017

benjamingr added a commit that referenced this pull request Dec 20, 2017

events: add off alias to removeListener
Add `off` as an alias for `removeListener`

PR-URL: #17156
Refs: #17102
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

@addaleax addaleax removed the author ready label Dec 29, 2017

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

events: add off alias to removeListener
Add `off` as an alias for `removeListener`

PR-URL: nodejs#17156
Refs: nodejs#17102
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

events: add off alias to removeListener
Add `off` as an alias for `removeListener`

PR-URL: nodejs#17156
Refs: nodejs#17102
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

@Ajido Ajido referenced this pull request Apr 25, 2018

Closed

doc: add emitter.off to events.md #20291

2 of 2 tasks complete

vsemozhetbyt added a commit that referenced this pull request Apr 25, 2018

doc: add emitter.off() to events.md
PR-URL: #20291
Refs: #17156
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

isurusiri added a commit to isurusiri/node that referenced this pull request Apr 30, 2018

doc: add emitter.off() to events.md
PR-URL: nodejs#20291
Refs: nodejs#17156
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins added a commit that referenced this pull request May 4, 2018

doc: add emitter.off() to events.md
PR-URL: #20291
Refs: #17156
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment