make event listener registration on instantiation consistent with EventEmitter

RESOLVED WONTFIX

Status

P2
normal
RESOLVED WONTFIX
8 years ago
8 years ago

People

(Reporter: myk, Unassigned)

Tracking

unspecified

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

8 years ago
Over in bug 588732, we're switching to the EventEmitter model for registering event listeners on objects provided by SDK modules.

I intend to retain the ability for consumers to register event listeners when instantiating objects via constructors, but the current mechanism for doing so, while consistent with the old model for registering event listeners on objects, is not so consistent with the EventEmitter model.

So we should come up with a new mechanism that is consistent with the EventEmitter model.

Currently, registering event listeners when instantiating objects looks like this:

  let thing = Thing({
    onFoo: function() { ... }
  });

That's consistent with the old model for registering event listeners:

  thing.onFoo.add(function() { ... });

But it isn't very consistent with the new model:

  thing.on("foo", function() { ... });

My initial thinking is to provide consumers with an "on" option whose value is a hash of "event-name: function" pairs, i.e.:

  let thing = Thing({
    on: { foo: function() { ... } }
  });

(Internally, implementations would call EventEmitter.on("foo") to register the event listeners in the "on" option just as they current call .add() to register the listeners specified via "onFoo"-style options.)

Thoughts?
(Reporter)

Updated

8 years ago
OS: Linux → All
Priority: -- → P2
Hardware: x86 → All
Target Milestone: -- → 0.8
Ugh.  Why are we changing the event interface?  (Yeah, I saw the bug.)

Given the new event interface, I think we should drop the ability to register listeners on construction.  Part of the rationale as I understand it for adopting the new interface was that it's more webby, and it's webby to have to register listeners on objects after you create them.  Let's go whole hog!  Go big or go home!  I guess it's also webby to have attributes named "onfoo", which here would translate as

  Thing({ onfoo: function () ... })

(lowercase "foo", single function only), but I've never liked that syntax.
(Reporter)

Comment 2

8 years ago
It's very convenient to be able to register event listeners on construction, particularly for use cases like:

  Request({
    url: "http://example.com/",
    /* event listener */
  }).get();

Hence I would like to retain the ability to do so.

I don't like the `onfoo` syntax either.  But in any case `onfoo` is webby for setting event listeners after instantiation, and EventEmitter is not going to allow setting `onfoo` after instantiation, so it doesn't seem like the optimal option name for adding event listeners during instantiation.
Sorry to jump in late on this but I don't see any problems with registering event listeners in the constructor with EventEmitter.

Have you looked at this diff http://gist.github.com/566773 that switches request module to the EventEmitter without any changes to the constructor options. It was submitted to the Bug 593737
Sorry I rushed replying in previous comment, after looking again at this again let me rephrase myself. I do think that there is any real necessity of changes to the option keys of constructor even though in the web onevent is used, listeners are not passed to the constructors anyway. I also do think that passing listeners to the constructor is very useful. I don't think that onEvent is less consistent with EventEmitter then onevent.
(Reporter)

Comment 5

8 years ago
The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: Trunk → unspecified
(Reporter)

Comment 6

8 years ago
We considered this and decided not to do it.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.