594129 - make event listener registration on instantiation consistent with EventEmitter

Status: RESOLVED WONTFIX

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Myk Melez [:myk] [@mykmelez]]

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?

Comment 1

[Drew Willcoxon :adw]

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.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

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

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

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.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

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".

Comment 6

[Myk Melez [:myk] [@mykmelez]]

We considered this and decided not to do it.