Level / abstract-level

Abstract class for a lexicographically sorted key-value database.
MIT License
128 stars 8 forks source link

Ditch EventEmitter #75

Open telamon opened 1 year ago

telamon commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 10 months ago

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 commented 8 months ago

+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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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.