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

Ditch EventEmitter #75

Open
telamon opened this issue Jul 30, 2023 · 8 comments
Open

Ditch EventEmitter #75

telamon opened this issue Jul 30, 2023 · 8 comments

Comments

@telamon
Copy link

telamon commented Jul 30, 2023

Hello, I imported package browser-level
and received the following error

✘ [ERROR] Could not resolve "events"

    node_modules/abstract-level/abstract-level.js:5:33:
      5 │ const { EventEmitter } = require('events')
        ╵                                  ~~~~~~~~

  The package "events" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "--platform=node" to do that, which will remove this error.

I feel having dependencies for node primitives in a meta-package such as abstract-level is overkill.

Would it be possible to move away from EventEmitters in favour for async generators or at worst a single function subscription pattern?

const unsubscribe = subscribe(event => ...)

I still dream nightmares about maintaining references to unremoved listener functions.

@vweevers
Copy link
Member

I feel having dependencies for node primitives in a meta-package such as abstract-level is overkill.

What's the real reason you're asking this question? Because abstract-level is not a meta-package (seeing as it does have code - but maybe we have a different definition of meta-package) and given that fact, I don't understand what you mean by overkill.

Could you please clarify? Thanks!

@telamon
Copy link
Author

telamon commented Oct 30, 2023

I think I was trying to say that the event emitter pattern has some downsides to cleanup/deallocation. Like there is no way to remove a listener without explicity keeping reference to the callback function. ES6 generators&iterators provide a modern elegant alternative for event streams, that is also dependency free.

Apologies for the term meta package, I didn't mean to say that it doesn't contain code.

I guess my real question is why is EventEmitter depended on internally when the public API encourages for await?

@vweevers
Copy link
Member

I think I was trying to say that the event emitter pattern has some downsides to cleanup/deallocation. Like there is no way to remove a listener without explicity keeping reference to the callback function.

If you're looking for sugar, use the on utility:

const { on } = require('events')

for await (const event of on(db, 'put')) {
  console.log(event) // [key, value]
}

As for doing that dependency-free, we might some day. The amount of reasons for having events (in any form) is slowly decreasing. For example, instead of db.once('open', fn) it's preferred to do await db.open(), and some events will be deprecated in abstract-level v2. I can't predict what will be left (or rather, I want that to be driven by ecosystem needs) and until we're there, I like the simplicity, flexibility and synchronicity of events.

I guess my real question is why is EventEmitter depended on internally when the public API encourages for await?

One part for historical reasons and backwards compat, and one part because events are synchronous, which is a better fit.

@telamon
Copy link
Author

telamon commented Jan 5, 2024

Yes your example provides de-initialization which is good. I am not familiar with level-db internals so i cannot say if all listeners attached must manually be cleared in order to let the garbage collector liberate resources. But I'm somewhat traumatized from hunting emitter.on() without a matching emitter.off() due the "set and forget"-approach most people are taught when interacting with events.
Which is why i recommend the explicit return of an bound unsubscribe function.

As for sugar no. I'd love to see smaller dependency footprints.
As mentioned we're using browser-level and it's a bit unfair that lighter devices has to process bigger dependency bundles due to shims for serverware-API instead of relying on cross-runtime API's.

I hope my concerns were received well, I enjoy working with abstract-level & browser-level but I wish the impact on the dependency tree was less.

@nichoth
Copy link

nichoth commented Mar 22, 2024

+1 replace 'events'

This is a node specific module, and it breaks building this for the browser. If you want to keep the event emitter interface, I would move to something in user-land, like nanoevents.

@vweevers
Copy link
Member

vweevers commented Mar 22, 2024

it breaks building this for the browser

I want to add some nuance to that. Webpack, Browserify, Vite and more are perfectly capable of bundling Level modules, although some make it painful by requiring configuration. It's unfortunate that the larger JavaScript ecosystem moved away from node shims because it removed the ability to reuse them. So now one module is using e.g. nanoevents and another is using Node.js events (directly or via the npm package events) and another uses mitt. More fragmentation, less collaboration, smaller bundles for one but bigger bundles for all.

The nanoevents package does significantly reduce size but it's not a fair comparison because it only has 2 functions, and not even once() for example. It's a code golfing exercise. Of course you can implement once() yourself like the readme demonstrates but that just shows every module now has to bring that code (and maintain it).

@jacoscaz
Copy link

jacoscaz commented Mar 22, 2024

Would it be possible to move away from EventEmitters in favour for async generators or at worst a single function subscription pattern?

In addition to @vweevers 's point, the definition of worst depends on what we're looking at. It's been a while since I've last done a thorough testing session but insofar as I am aware, callback-based patterns (through EventEmitter-like structures or single function subscription) are significantly more performant than native async iteration. Furthermore, both async iteration and single function subscription make multi-subscriber use cases harder to implement.

It would be nice to not have to shim node:events with a userland alternative, though it's really easy to do so, but performance and maintainability costs should also be taken into account when considering such a transition.

@nichoth
Copy link

nichoth commented Mar 22, 2024

True about factoring things for deduplication in a broader context. As a working solution, I have installed the npm module events, as it seems the closest thing to a standard in user-land implementations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants