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

add off as an alias for removeListener #17102

Closed
ianstormtaylor opened this issue Nov 17, 2017 · 6 comments
Closed

add off as an alias for removeListener #17102

ianstormtaylor opened this issue Nov 17, 2017 · 6 comments
Labels
events Issues and PRs related to the events subsystem / EventEmitter. feature request Issues that request new features to be added to Node.js.

Comments

@ianstormtaylor
Copy link
Contributor

Hello!

I'd like to open an issue to advocate for adding an off method to EventEmitter as an alias of removeListener, to parallel the existing on alias for addListener.

I know that this has been opened a few times in the past (on old node.js and on io.js), but many of the issues were a long time ago, the landscape has converged a lot more on the off method since then, and I'd like to argue it once more with a well-researched issue. Thank you!

This addition would reduce the existing UX confusion that results from missing the very popular and commonly implemented parallel method to on. It would provide a nice UX improvement by giving the listener removal use case its own terse method. And it would help to improve interoperability with other very common event emitter libraries in node.js and in the larger Javascript world.

Popular Libraries

The on/off method combination is very popular, and it's used by some of the most popular libraries:

It's even exposed by popular node.js libraries too, like Socket.io and Primus which choose to bundle their own event emitters with the off method.

Having it in these very popular libraries has contributed over time to people coming to expect off as the parallel method to on. Not having it in node.js creates confusion (links to this coming up!) and reduces interoperability with other libraries.

Existing Emitters

Further, if you look at user-land event emitter libraries in NPM, every single one with over 1 million downloads per month implements the off method...

  • component-emitter
    • 14.5m downloads / month
    • Used by libraries like superagent, socket.io, analytics.js, ...
  • eventemitter3
    • 8.4m downloads / month
    • Used by libraries like primus, http-proxy, quill, reflux, ...
  • eventemitter2
    • 4.1m downloads / month
    • Used by libraries like pm2, grunt, ...
  • tiny-emitter
    • 1.3m downloads / month
    • Used by libraries like mathjs, clipboard, es6-map, es6-set, ...

Even I was a little surprised that it was this popular. It's so commonplace that not a single emitter library above 1 million downloads a month does not implement off. Which results in many, many other popular node.js libraries exposing off in their own emitters and inherited classes.

That doesn't even include the many other event emitter popular libraries that have implemented off as well, like wolfy87-eventemitter, event-lite, co-events, event-pubsub, component-emitter2, event-emitter-es6, ...

Or other libraries like p-event that handle the off method as a first-class property of emitters for compatibility since so many people use it.

There are even libraries like nee-off, emitter-mixin or events-off which have been created to add in the off method to existing event emitters that haven't implemented it.

UX Confusion

As you can see from above, the on/off pattern is very popular. It's intuitive, it has parallel structure, and it's very terse, which is nice since it's so often used and mixed in to other APIs.

But right now node.js only implements the on method of the duo, which leads to confusion from anyone who's used these other popular libraries. Confusion like in this StackOverflow question.

Question: ... How does one turn off the callback to prevent further on message callbacks? There happens to be no off method. I have to update the callback with a new one and it appears that all the old callbacks are being triggered as well.

cluster.on('fork', worker => {
  worker.on('message', msg => {// Do something...})
})

Answer: Why they added .on() as an alias, but did not add .off() as an alias too, I don't really know (seems logical to me). ...

This kind of confusion doesn't only happen in the node.js world. You can see the same confusion in other libraries that implement on without the off parallel, like in Angular.

To people who are trying to avoid the confusion in their own codebase, they actually end up having to stop using the on method, because it's so easy to forget that off doesn't exist as its parallel—like these folks did.

Since this is easy to miss, would you mind changing our .on()s to .addListener() so we aren't tempted to .off() elsewhere?

Not having this parallel even leads to some weird differences between the browser and node environments. For example, Socket.io instances have the off method client-side, but don't have it server-side, because the node.js core EventEmitter doesn't implement it.

Previous Issues

This issue has been brought up a few, different times before.

But I think a decent amount has changed since then, specifically the amount of other big libraries that have implemented the same pattern, and the amount of userland support for the on/off combination.

Here are some of the old arguments against adding off...

"Off isn't the parallel"

One of the arguments against using off was that the word "off" in English is not always parallel to "on". (It was once pointed out that "on" could have an antonym of "under" too.)

But I think this is super pedantic, and doesn't actually hold that much water. All of the existing support for on/off as a pair, and the StackOverflow questions that prove people immediately jump to "off" as the antonym prove this. And at this point the amount of support for the combination is only further reinforcing this assumption, which leads to people getting confused.

@domenic: It's a pretty common pattern in other event-related libraries (EventEmitter2 and jQuery/Zepto being the biggest I can think of). Coming from client-side, I tried off first, and when it didn't work, had to go consult the docs to find removeListener.

If you really do want to get into the weeds there, there are arguments in favor of it being parallel like:

@ljharb: 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.

But I think (and @ljharb agreed that) this descends into overly pedantic discussions. Instead, the vast amount of support for on/off as a combination should prove already that it makes sense to a lot of people, and is widely liked.

Not only that, but if you want an equally not that useful argument, if you lookup "on" on thesaurus.com the only antonym that even shows up is "off". 😄

"Aliases are bad"

Another of the arguments against it was that aliases are bad, and that on should be removed instead of adding off.

But this is obviously not a solution. There are so many dependencies on on that it's not going to be removed, and it shouldn't be.

People like the terseness of on a lot, and would appreciate the same terseness in off. For proof, just look at the node.js documentation. Even though on is technically the alias and not the canonical method name, almost all every single code snippet in the node.js docs uses on instead of addListener—that's because people appreciate the terseness it gives you.

"Removing is rare"

Another argument that was brought up against off is that using removeListener is rare, so it doesn't merit having an alias.

I think this isn't valid on two counts.

First, in the case of a use case where two methods have a very clear parallel, I don't think it makes sense to omit one of the methods in the pair just because it is used slightly less. The pair should be treated as a single "feature". If the alias for on is necessary (which I think it is given its broad usage), then the alias for off should be paired with it. That's how well-designed APIs accommodate users with a good UX. Otherwise you set people up for an expectation that isn't met.

@ljharb: 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.

Second, although using off may be less common for the node.js core APIs, because you're often setting up listeners for the duration of the process, one of the main purposes of EventEmitter is to be inherited from by userland libraries. And at that point, I don't think we should assume the same usage patterns as for the core APIs. It's very possible there are libraries like Socket.io, or browser use cases like unmounting components that use off much more frequently. It's no longer reasonable to force the same usage pattern on everyone else, especially for something this simple.

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

Previous PR

The good news is actually that there is already a pull request for this that we can just re-open if we decide it's something that should be added. (Obviously I think it is 😄)

#540

And as you can see, it's pretty minimal.

So I'd like to propose that the existing PR be reopened and merged, or we can resubmit a new one if it conflicts too much with the current codebase.

That's it! Thank you for reading this far.

I'd love to hear what you think in the comments.

If anyone else has more examples to add to any of the lists, please let me know. I'm going to cc a few people who had brought this issue up previously — @ljharb @ChALkeR @domenic @yosuke-furukawa

@mscdex mscdex added events Issues and PRs related to the events subsystem / EventEmitter. feature request Issues that request new features to be added to Node.js. labels Nov 17, 2017
@ljharb
Copy link
Member

ljharb commented Nov 17, 2017

This would be excellent to add. Airbnb currently has to monkeypatch this in.

@daxlab
Copy link
Contributor

daxlab commented Nov 19, 2017

Can I pick this up, in case we need a new PR ?

@tniessen
Copy link
Member

@daxlab I am sorry, seems like @Ulmanb opened #17156 already. We will still give preference to #540 if @yosuke-furukawa decides to reopen it.

@tniessen
Copy link
Member

If you are interested in this issue, feel free to vote here: #17156 (comment)

benjamingr pushed a commit that referenced this issue Dec 20, 2017
Add `off` as an alias for `removeListener`

PR-URL: #17156
Refs: #17102
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@benjamingr
Copy link
Member

Landed :) Thanks for the suggestion and for sticking through for the discussions @ianstormtaylor. The contribution is appreciated.

The API should be available in Node v10.

@ianstormtaylor
Copy link
Contributor Author

Thank you @benjamingr!!

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. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants