There are at least 3 event systems sprinkled throughout Dev Tools, each with their own slightly different APIs: * event-emitter * sdk/event/core * dbg-client's eventSource We should choose one and standardize on it to reduce confusion.

For future reference, event-emitter has several more features (or differences at least...) compared to SDK event/core: * e-e listeners get the event name, which can be used to have one "master" listener for many events (we use this in a few places) * e-e's off() is able to remove one-time listeners * e-e.decorate(), which is quite elegant, has no direct equivalent * Bug 974056 will add debug logging to e-e We'll want to add these to event/core if we standardize on that one.

+1 for e-e

One more benefit: you can log e-e events with the `devtools.dump.emit` pref.

+1 on using event-emitter.

(In reply to J. Ryan Stinnett [:jryans] (on PTO Feb. 28 - Mar. 7) from comment #1) > For future reference, event-emitter has several more features (or > differences at least...) compared to SDK event/core: > > * e-e listeners get the event name, which can be used to have one "master" > listener for many events (we use this in a few places) This can be done with `on(target, "*", function(name, data) { })` where name is the event name. > * e-e's off() is able to remove one-time listeners Just made bug 1137930 for this on sdk/event/core > * e-e.decorate(), which is quite elegant, has no direct equivalent Not sure what this does atm. > * Bug 974056 will add debug logging to e-e Just made bug 1137929 for this on sdk/event/core > We'll want to add these to event/core if we standardize on that one. I think we want to add most of these features either way, so perhaps e-e can be used and we can have it reuse sdk/event/core while we make sdk/event/core more useful.

As an addon developer, I see a lot of value in having Firefox and SDK developers using the same event library, because it would be make one strong library for both consumers. I don't really care which is it, but I believe it should be the same. I think what I'm asking for argues in favor of sdk/event/core, but it might be possible to deprecate it and replace it with e-e if there are strong arguments in its favor.

I like removing code, and it will be easier to remove e-e than sdk events. So I'd prefer to beef up sdk's as needed and dump e-e.

I've been running into this for awhile now. Things I miss in e-e from the sdk/core/events: * sdk/event/utils has a `pipe` method that redirects messages from one event source to another. * sdk/event/core also has a "*" listener, like Erik mentioned, which is great, especially along with pipes. This would let us have multiple events on the same handler. * sdk/event/target is a barebones class, EventTarget, that has all the on/off/etc methods for events, like EventEmitter, but based on sdk/core/heritage#Class, and provides no decorator helper like EE does. +1 for sdk events if we expose a `decorate` function from sdk/event/target, which would get us the same results as e-e, except being backed by the SDK, so giving us the ability to pipe, wildcards, and other event utils if we need. Downside is the work necessary to no longer need the first argument of the event name in e-e implementations. sdk events won't be going anywhere. If there's an agreement on what to do for this, I'll gladly jump on this grenade. This is promises all over again

David's and Dave's arguments have merit, so I'm changing my +1 from e-e to using the SDK events.

Yes, I'm fine with SDK events too assuming we add the missing features, mainly: * Debug event logging * Some kind of |decorate| equivalent, so that it's not necessary to use sdk/core/heritage to extend EventTarget In the past I thought Alex had said sdk/event/core depended on lots of SDK modules and would lead to memory bloat, but looking again it seems like it's only sdk/event/target that starts getting expensive, so let's avoid that one if we can. As Jordan mentions, the main conversion headache is the arguments changing in event handlers, which is probably a silent error in many cases, but we'll have to tackle it one way or another to do the conversion.

It'd be great if the SDK's `once` returned a promise. That's a big convenience for testing, and otherwise we will require workarounds like the part 1 of the patch in 1120833.

(In reply to Brian Grinstead [:bgrins] from comment #11) > It'd be great if the SDK's `once` returned a promise. Same with the `once` method attached to EventTarget. This looks like it could be a backwards-compatibility issue though, since it currently returns `this` via the chainable call. Could that behavior be changed directly on EventTarget? Could if be changed if the functionality was added via the decorate call from Comment 8?

A few updates in the year or so since the last comments here: * event-emitter is now imported as a JSM in various Firefox UI modules, so it's being used in non-DevTools * For devtools.html, it feels simpler to have our event library in /devtools tree

Bug 1350645 tracks removal of various SDK APIs throughout DevTools, including sdk/event/core. This would suggest we might want to head towards event-emitter instead (or else we'd be copying sdk/event/core to DevTools).

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #8) > * sdk/event/utils has a `pipe` method that redirects messages from one event > source to another. > * sdk/event/core also has a "*" listener, like Erik mentioned, which is > great, especially along with pipes. This would let us have multiple events > on the same handler. For what it's worth, these are two of the reasons that the SDK's even system adds so much overhead. In particular with multiple layers of pipe() calls, we wind up with huge call stacks that are difficult to debug, and add up enough to significantly slow things down.

Reading back through the comments, it feels like many of the arguments in support of SDK events assumed "SDK is going to be around, so let's make use of it", but this part that's no longer true. I've been a fan of event-emitter myself, and especially with SDK perf concerns plus SDK code removal, it continues to seem like the right fit to me. Perhaps we can discuss more at the next DevTools meeting.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16) > I've been a fan of event-emitter myself, and especially with SDK perf > concerns plus SDK code removal, it continues to seem like the right fit to > me. I don't want to keep the SDK events around, however I'd like to keep the same flexibility the API provides. Basically everything the event-emitter does can be done by the SDK events but not vice versa. So I'd like to fill the gap. Specifically, I'd like to have a core API as we have in SDK: `on`, `once`, `off`, and `emit` that takes as first argument the object (that also would makes "decorate" cleaner), leaving the possibility to have a more functional approach. The EventEmitter can be based on that and the functionality kept the same. That also helps to have cleaner API when we don't want to have a full emitter, but just a "receiver" object (a kind of event bus). Plus, I'd like the fact that the `off` function in SDK can remove all the listeners on a given object / emitter if no listener is given: that helps in certain situation so that we don't need to keep a reference to a listener just for the sake of future removal. There is also the `setListeners` utility function that I think it should be ported.

I think this is a particularly bad idea, and is in fact one of the reasons this API is one of my highest priorities for removal. WeakMaps are great, but they're not especially cheap. Storing listeners directly on an EventEmitter instance is a simple, JITtable shape lookup. Storing them in a WeakMap is a hash lookup, which has non-trivial memory, CPU, and GC overhead. We use event emitters *a lot*, and that overhead starts to add up quickly. There are also weird issues with prototype inheritance when that approach is used. The SDK used to try to get around that by creating a namespace object that inherited from all of the objects in the prototype chain. But aside from being expensive, that led to even weirder issues where events would be registered or dispatched only on some objects depending on which object in the prototype chain the event was first referenced on.

(In reply to Kris Maglione [:kmag] from comment #18) > I think this is a particularly bad idea, and is in fact one of the reasons > this API is one of my highest priorities for removal. > WeakMaps are great, but they're not especially cheap. I wasn't actually talking about implementation's details here; and I wasn't actually thinking to use WeakMaps. If we agreed on those API, my idea was actually using a `Symbol` for the listeners. So, it will end up to add the listeners directly to the object anyway. Of course there are downside to that too, but I think it's a better option than what we have currently in both SDK and EventEmitter. > There are also weird issues with prototype inheritance when that approach is > used. The SDK used to try to get around that by creating a namespace object > that inherited from all of the objects in the prototype chain. Yeah, the namespace thingy wasn't so great. It was created at that time to fill a gap in the language itself. Luckily this is not necessary anymore.

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #19) > I wasn't actually talking about implementation's details here; and I wasn't > actually thinking to use WeakMaps. > If we agreed on those API, my idea was actually using a `Symbol` for the > listeners. So, it will end up to add the listeners directly to the object > anyway. Of course there are downside to that too, but I think it's a better > option than what we have currently in both SDK and EventEmitter. This is essentially what we get with EventEmitter, anyway, except that we only have to worry about it once per object, and we don't run the risk of attaching the symbol to objects where we aren't expecting it. I'll admit that it would be nice not to have to add separate on/off/once/emit methods to every object that we call `decorate()` on, as opposed to subclassing EventEmitter, but I'd still rather that than have the property check and possible shape change every time we call on/off/once/emit on any object. A version of decorate() that could be called on prototypes would improve that situation, at least.

(In reply to Kris Maglione [:kmag] from comment #20) > (In reply to Matteo Ferretti [:zer0] [:matteo] from comment #19) > > I wasn't actually talking about implementation's details here; and I wasn't > > actually thinking to use WeakMaps. > > If we agreed on those API, my idea was actually using a `Symbol` for the > > listeners. So, it will end up to add the listeners directly to the object > > anyway. Of course there are downside to that too, but I think it's a better > > option than what we have currently in both SDK and EventEmitter. > This is essentially what we get with EventEmitter, anyway, except that we > only have to worry about it once per object, and we don't run the risk of > attaching the symbol to objects where we aren't expecting it. But that's what symbol are for, IMVHO. Having this symbol and functions that works with that, permits to have the same functionality we have with the EventEmitter and the SDK: in most of the case, we'll work with the `EventEmitter`, so nothing changes. But, if we want to ensure a nicer encapsulation and don't have everything on the same object (emitting and receiving), that would be possible too, and we keep the compatibility between EventEmitter and such methods, something we don't have now. Plus, that would makes much easier the current transition with the devtools code that is actually using the mixing approach of EventEmitter and SDK events. > I'll admit that it would be nice not to have to add separate > on/off/once/emit methods to every object that we call `decorate()` That's another improvement I'd like to have too. As soon as we can use ES6 classes, when we're going to refactor part of the devtools code, we could just subclassing EventEmitter. In the meantime, the decoration will be cleaner using the `Symbol` and functions rather than attaching the methods and properties in the way is now. > opposed to subclassing EventEmitter, but I'd still rather that than have the > property check and possible shape change every time we call on/off/once/emit > on any object. I'm not suggesting that. I'm not saying that we should use the core method on any kind of arbitrary objects. What I'm suggesting is having core methods that uses a Symbol instead of a plain property like `_eventEmitterListeners`. The `EventEmitter` will be just based on those methods. But since we have this nice separation, we could also create object that are just receiver, for example. No shape change would be done here, especially not every time we call the methods above.

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #21) > But that's what symbol are for, IMVHO. Having this symbol and functions that > works with that, permits to have the same functionality we have with the > EventEmitter and the SDK: in most of the case, we'll work with the > `EventEmitter`, so nothing changes. But, if we want to ensure a nicer > encapsulation and don't have everything on the same object (emitting and > receiving), that would be possible too, and we keep the compatibility > between EventEmitter and such methods, something we don't have now. Symbols are for a lot of things, but they're mostly meant to be things that you attach to your objects yourself, rather than for someone else to attach to your objects. E.g., they're used to specify iteration behavior, or stringification, or they can be used to attach a property to a generic base class without conflicting with subclass. But attaching them to objects that aren't expecting to have them attached is a different matter. They're still enumerable unless you take care for them not to be. They can affect structured clone compatibility. And, importantly, when you attach them, you change the shape of the object. When we decorate an object with an EventEmitter from the constructor, or just inherit from the class, all instances of the class wind up with the same shape chain. If we add the property the first time we happen to attach an event to it, we can wind up with any number of different shape chains for instances depending on when it was first added, and that can add up to a lot of wasted memory. I'm not normally a fan of rushing to micro-optimize things like this, but when we're thinking about the best design for an API as widely-used as this one, I think it matters. > That's another improvement I'd like to have too. As soon as we can use ES6 > classes, when we're going to refactor part of the devtools code, we could > just subclassing EventEmitter. > In the meantime, the decoration will be cleaner using the `Symbol` and > functions rather than attaching the methods and properties in the way is now. I think even without ES6 classes, we can make this work fairly cleanly. E.g., function Thing() { EventEmitter.call(this); } EventEmitter.inherit(Thing); // ... const EVENT_LISTENERS = Symbol("EventEmitter.@@event_listeners"); function EventEmitter() { this[EVENT_LISTENERS] = {}; } EventEmitter.inherit = function (cls) { for (let method of Object.getOwnPropertyNames(EventEmitter.prototype)) cls.prototype[method] = EventEmitter.prototype[method]; } > > opposed to subclassing EventEmitter, but I'd still rather that than have the > > property check and possible shape change every time we call on/off/once/emit > > on any object. > > I'm not suggesting that. I'm not saying that we should use the core method > on any kind of arbitrary objects. > What I'm suggesting is having core methods that uses a Symbol instead of a > plain property like `_eventEmitterListeners`. The `EventEmitter` will be > just based on those methods. OK. We probably agree on that, then.

Marco, I'm removing this bug for the NoSDK project, since this meta includes also `eventSource` that is not part of NoSDK scope.

Should still be open? it looks like it's dependencies are finished and we seem to be using the new event emitter almost everywhere.

(In reply to Yulia Startsev [:yulia] from comment #24) > Should still be open? it looks like it's dependencies are finished and we > seem to be using the new event emitter almost everywhere. It looks like bug 1042642 (about `eventSource`) is still a valid issue, so I have reopened that. I believe that is the only remaining task under this meta-bug.