| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
Changes LGTM - let's see if we have consensus on adding it.
Sorry, something went wrong.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Agree on preferring the original PR if possible, psyched to see this happen!
Sorry, something went wrong.
|
Please make sure to CITGM this. |
Sorry, something went wrong.
|
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1095/ |
Sorry, something went wrong.
|
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… |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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 And any combination of (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! |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@devsnek addListener exists, it's just more verbose so no one uses it in core. |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
I'm curious where else off is used as opposite of on, since front-end is being cited. |
Sorry, something went wrong.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Front-end tooling is currently (very likely) the largest consumer of Node.js... this isn't back-end development.
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. |
Sorry, something went wrong.
|
@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.) |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
@MylesBorins for me it was not about timing but about having a conclusion. I am aware that this is not time sensitive. |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
I am going to abstain, there is already a majority anyway though. |
Sorry, something went wrong.
|
+1 from me. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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>
<!-- Have any questions? Check out the contributing docs at https://gatsby.app/contribute, or ask in this Pull Request and a Gatsby maintainer will be happy to help :) --> ## Description <!-- Write a brief description of the changes introduced by this PR --> This fixes a bug introduced in #10593 by replacing the `.off` call with `.removeListener`. `.off` was introduced in Node v10.0.0 as an alias for `.removeListener` (nodejs/node#17156). ## Related Issues <!-- Link to the issue that is fixed by this PR (if there is one) e.g. Fixes #1234, Addresses #1234, Related to #1234, etc. --> Related to #10612
<!-- Have any questions? Check out the contributing docs at https://gatsby.app/contribute, or ask in this Pull Request and a Gatsby maintainer will be happy to help :) --> ## Description <!-- Write a brief description of the changes introduced by this PR --> This fixes a bug introduced in gatsbyjs#10593 by replacing the `.off` call with `.removeListener`. `.off` was introduced in Node v10.0.0 as an alias for `.removeListener` (nodejs/node#17156). ## Related Issues <!-- Link to the issue that is fixed by this PR (if there is one) e.g. Fixes gatsbyjs#1234, Addresses gatsbyjs#1234, Related to gatsbyjs#1234, etc. --> Related to gatsbyjs#10612
<!-- Have any questions? Check out the contributing docs at https://gatsby.app/contribute, or ask in this Pull Request and a Gatsby maintainer will be happy to help :) --> ## Description <!-- Write a brief description of the changes introduced by this PR --> This fixes a bug introduced in gatsbyjs#10593 by replacing the `.off` call with `.removeListener`. `.off` was introduced in Node v10.0.0 as an alias for `.removeListener` (nodejs/node#17156). ## Related Issues <!-- Link to the issue that is fixed by this PR (if there is one) e.g. Fixes gatsbyjs#1234, Addresses gatsbyjs#1234, Related to gatsbyjs#1234, etc. --> Related to gatsbyjs#10612
| Back | FazBrowse Home | New Git URL |
Checklist
Hi, fixes #17102 - this is my first issue #goodnessSquad