593921 - consider passing |this| object as first argument to callback functions

Status: RESOLVED FIXED

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

Description

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

Event callback functions in high-level modules of the SDK are conventionally called with the |this| object set to a specific value in order to prevent the |this| object from being set to the global object of the calling context, which might expose private information in the module from which the call originates.

In order to take advantage of this behavior to simplify the API, I established the second convention that the |this| object in such callbacks should be the object on which the callback is defined.  For example, the |this| object in Request.onComplete callbacks is the Request instance.

I further established the third convention that callback arguments should not include the |this| object, since it is unnecessary and duplicative to provide it as an argument.

I think the first and second conventions are still the right thing to do.  We have to provide a |this| object to prevent leaking private information, and the object on which the callback is defined still seems like the most intuitive object to set as the |this| object in such functions.

The third convention, however, might become problematic, because ES5's new Function.bind() method makes it easy to bind a function to an object in a way that prevents us from setting the |this| object to anything else when we call it.

And if developers use it on callbacks, for which there are interesting use cases (like a Request.onComplete callback bound to an object that caches the results of multiple Requests), then the third convention will either confound their expectations or frustrate them.

It might even cause them to wrap a bound callback in an anonymous function just to convert |this| into an argument to that callback, which would be ironic, since avoiding the need to wrap callbacks in anonymous functions is one of the primary use cases for Function.bind().

This is certainly speculative, since Function.bind() is not in such wide use today (although Prototype's equivalent <http://www.prototypejs.org/api/function/bind> may be).  But given that it is to become a part of the core language, I expect usage to grow.  So the third convention concerns me.

The obvious solution would be to change the convention to require that objects pass the |this| object as the first argument to callbacks.  This makes callback interfaces more complicated.  But it supports Object.bind().  And it may have the beneficial side-effort of making it more obvious how to access the |this| object (which may not always be clear to developers unfamiliar with the second convention).

Thoughts?

Comment 1

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

(In reply to comment #0)
> Event callback functions in high-level modules of the SDK are conventionally
> called with the |this| object set to a specific value in order to prevent the
> |this| object from being set to the global object of the calling context, which
> might expose private information in the module from which the call originates.
> 
> In order to take advantage of this behavior to simplify the API, I established
> the second convention that the |this| object in such callbacks should be the
> object on which the callback is defined.  For example, the |this| object in
> Request.onComplete callbacks is the Request instance.
> 
> I further established the third convention that callback arguments should not
> include the |this| object, since it is unnecessary and duplicative to provide
> it as an argument.
> 
> I think the first and second conventions are still the right thing to do.  We
> have to provide a |this| object to prevent leaking private information, and the
> object on which the callback is defined still seems like the most intuitive
> object to set as the |this| object in such functions.
> 
> The third convention, however, might become problematic, because ES5's new
> Function.bind() method makes it easy to bind a function to an object in a way
> that prevents us from setting the |this| object to anything else when we call
> it.

I'm bit confused, did you meant 2nd convention instead ??  

> 
> And if developers use it on callbacks, for which there are interesting use
> cases (like a Request.onComplete callback bound to an object that caches the
> results of multiple Requests), then the third convention will either confound
> their expectations or frustrate them.
> 
> It might even cause them to wrap a bound callback in an anonymous function just
> to convert |this| into an argument to that callback, 

I personally had this frustration and worked around it in several places already.

> which would be ironic,
> since avoiding the need to wrap callbacks in anonymous functions is one of the
> primary use cases for Function.bind().
> 
> This is certainly speculative, since Function.bind() is not in such wide use
> today (although Prototype's equivalent
> <http://www.prototypejs.org/api/function/bind> may be).  But given that it is
> to become a part of the core language, I expect usage to grow.  So the third
> convention concerns me.

NIT: Function.prototype.bind was considered by the ES comity only because of it's popularity that was established by prototype framework.  

> 
> The obvious solution would be to change the convention to require that objects
> pass the |this| object as the first argument to callbacks.  This makes callback
> interfaces more complicated.  But it supports Object.bind().  And it may have
> the beneficial side-effort of making it more obvious how to access the |this|
> object (which may not always be clear to developers unfamiliar with the second
> convention).
> 
> Thoughts?

I'm sorry I confused with a numbering of conversions, so I thought I'll just express my thought what would be the ideal API from my point of view.

I think that setting |this| in the callbacks / listeners to the certain object is indeed necessary and I think it should be set null, for several reasons:

1) This matches the way it would behave in es5 "strict mode".
2) If callback.bind(...) was provided as a callback this will point to whatever it was bounded, since callback.bind(....).apply(....) doesn't changes 'this'.

object have to be passed as an argument instead of binding it to 'this' because:

1) It doesn't leads to a frustrating workarounds with closures.
2) It makes much better sense in certain API's like sidebar, page-mod, where binding symbiont to the this will only lead to confusions:
More details are here: https://bugzilla.mozilla.org/show_bug.cgi?id=593913
3) No matter how well new language feature is adopted at the moment it specifically was designed to address this issue and I think it's good idea to stick with it. (BTW it's better for performance as well cause bind does not generates nor new function nor a new lexical scope)

Comment 2

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

(In reply to comment #1)
> > The third convention, however, might become problematic, because ES5's new
> > Function.bind() method makes it easy to bind a function to an object in a
> > way that prevents us from setting the |this| object to anything else when
> > we call it.
> 
> I'm bit confused, did you meant 2nd convention instead ??  

No, I meant the third convention, that callback arguments should not include the |this| object (i.e. Request.onComplete should not pass the request object as an argument to the callback).

I think the second convention, to set the |this| object in callbacks to the object on which the callback is defined, is fine, even though it gets overridden for bound callbacks.


> I think that setting |this| in the callbacks / listeners to the certain object
> is indeed necessary and I think it should be set null, for several reasons:
> 
> 1) This matches the way it would behave in es5 "strict mode".

As I understand it, it is not possible to set the |this| object to null in strict mode, i.e. |(function(){}).call(null)| will throw an exception in strict mode.


> 2) If callback.bind(...) was provided as a callback this will point to whatever
> it was bounded, since callback.bind(....).apply(....) doesn't changes 'this'.

That's true, and it's what developers will expect if they have bound a callback, but it doesn't necessarily mean they will expect |this| to be null if they haven't bound a callback.

Once we switch to the EventEmitter model, there will be two ways to add an event listener to an object.  First, for objects that consumers construct, it will be possible to pass an event listener as an option to the constructor (although we might consider different syntax that aligns better to the new EventEmitter syntax):

  let request = new Request({
    onComplete: function() { ... }
  });

Second, for all objects, it will be possible to pass an event listener as an argument to the `on` method:

  request.on("complete", function() { ... });

In both cases, setting the |this| object to the object on which the callback is defined seems reasonably in line with developer expectations (and is consistent and idiomatic for non-bound callbacks, so easy to learn and remember), although I could understand the case for consistently setting it to, say, an empty object.


> 2) It makes much better sense in certain API's like sidebar, page-mod, where
> binding symbiont to the this will only lead to confusions:
> More details are here: https://bugzilla.mozilla.org/show_bug.cgi?id=593913

I don't understand to which confusion you are referring.  Can you elaborate?


> 3) No matter how well new language feature is adopted at the moment it
> specifically was designed to address this issue and I think it's good idea to
> stick with it. (BTW it's better for performance as well cause bind does not
> generates nor new function nor a new lexical scope)

I don't really see how this will have a measurable performance impact, and I think we should be very wary of making decisions that prematurely optimize for performance.

Nevertheless, I am inclined to agree with you that we should be passing the object on which an callback is defined as an argument to the callback, hence this bug.

Comment 3

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

> No, I meant the third convention, that callback arguments should not include
> the |this| object (i.e. Request.onComplete should not pass the request object
> as an argument to the callback).
> 
> I think the second convention, to set the |this| object in callbacks to the
> object on which the callback is defined, is fine, even though it gets
> overridden for bound callbacks.
> 

Ok here is my example:

const { HiddenFrame } = require("hidden-frame");
function MyConstructor() {
  this.onReady = this.onReady.bind(this);
  this.frame = HiddenFrame(this);  
}
MyConstructor.prototype.onReady = function() {
 this.frame.element // throws cause HiddenFrame did not returned yet
 this.element // undefined cause it's instance of MyConstructor
 // how do I get element ???
}


> 
> That's true, and it's what developers will expect if they have bound a
> callback, but it doesn't necessarily mean they will expect |this| to be null if
> they haven't bound a callback.
> 
> Once we switch to the EventEmitter model, there will be two ways to add an
> event listener to an object.  First, for objects that consumers construct, it
> will be possible to pass an event listener as an option to the constructor
> (although we might consider different syntax that aligns better to the new
> EventEmitter syntax):
> 
>   let request = new Request({
>     onComplete: function() { ... }
>   });
> 
> Second, for all objects, it will be possible to pass an event listener as an
> argument to the `on` method:
> 
>   request.on("complete", function() { ... });
> 
> In both cases, setting the |this| object to the object on which the callback is
> defined seems reasonably in line with developer expectations (and is consistent
> and idiomatic for non-bound callbacks, so easy to learn and remember), although
> I could understand the case for consistently setting it to, say, an empty
> object.
> 

Problem is illustrated above. if callback is bounded and object with actual value was passed as this there is no way to access it from the callback unless it's async.

> 
> > 2) It makes much better sense in certain API's like sidebar, page-mod, where
> > binding symbiont to the this will only lead to confusions:
> > More details are here: https://bugzilla.mozilla.org/show_bug.cgi?id=593913
> 
> I don't understand to which confusion you are referring.  Can you elaborate?
> 

Here is snippet you wrote in the mentioned bug 593913.

let pageMod = require("page-mod").PageMod({ ... });
  pageMod.on("ContentScriptStart", function(port) {
    port.postMessage("Hello, content script!");
    port.on("message", function(message) {
      console.log("A content script says what? " + message);
    }
  };

Now lets rewrite it with current proposal

let pageMod = require("page-mod").PageMod({ ... });
  pageMod.on("ContentScriptStart", function() {
    // this is not pageMod as one would expect it is port instead!!
    this.postMessage("Hello, content script!");
    this.on("message", function(message) {
      console.log("A content script says what? " + message);
  }
};

> 
> > 3) No matter how well new language feature is adopted at the moment it
> > specifically was designed to address this issue and I think it's good idea to
> > stick with it. (BTW it's better for performance as well cause bind does not
> > generates nor new function nor a new lexical scope)
> 
> I don't really see how this will have a measurable performance impact, and I
> think we should be very wary of making decisions that prematurely optimize for
> performance.
> 
> Nevertheless, I am inclined to agree with you that we should be passing the
> object on which an callback is defined as an argument to the callback, hence
> this bug.

Comment 4

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

> Ok here is my example:
>
> const { HiddenFrame } = require("hidden-frame");
> function MyConstructor() {
>   this.onReady = this.onReady.bind(this);
>   this.frame = HiddenFrame(this);  
> }
> MyConstructor.prototype.onReady = function() {
>  this.frame.element // throws cause HiddenFrame did not returned yet
>  this.element // undefined cause it's instance of MyConstructor
>  // how do I get element ???
> }

If I understand this correctly, HiddenFrame accepts an onReady callback, and the MyConstructor constructor provides it with MyConstructor.prototype.onReady, which it binds to the new instance of MyConstructor.

What I am suggesting in comment 0 is to have HiddenFrame pass its onReady callback a reference to the HiddenFrame instance, making the example look like the following:

  const { HiddenFrame } = require("hidden-frame");
  function MyConstructor() {
    this.onReady = this.onReady.bind(this);
    this.frame = HiddenFrame(this); 
  }
  MyConstructor.prototype.onReady = function(frame) {
    // frame is the element
  }


> Problem is illustrated above. if callback is bounded and object with actual
> value was passed as this there is no way to access it from the callback unless
> it's async.

We might be misunderstanding each other.  If I understand you correctly, I filed this bug in order to think about and potentially solve exactly the problem you are describing.  And my suggested change in comment 0 is intended to solve that problem.


> Here is snippet you wrote in the mentioned bug 593913.
>
> let pageMod = require("page-mod").PageMod({ ... });
>   pageMod.on("ContentScriptStart", function(port) {
>     port.postMessage("Hello, content script!");
>     port.on("message", function(message) {
>       console.log("A content script says what? " + message);
>     }
>   };
>
> Now lets rewrite it with current proposal
>
> let pageMod = require("page-mod").PageMod({ ... });
>   pageMod.on("ContentScriptStart", function() {
>     // this is not pageMod as one would expect it is port instead!!
>     this.postMessage("Hello, content script!");
>     this.on("message", function(message) {
>       console.log("A content script says what? " + message);
>   }
> };

Um, no.  What I am suggesting is that we would make it:

  let pageMod = require("page-mod").PageMod({ ... });
  pageMod.on("ContentScriptStart", function(pageMod, port) {
    // this is pageMod
    port.postMessage("Hello, content script!");
    port.on("message", function(message) {
      console.log("A content script says what? " + message);
  });

Comment 5

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

Sorry maybe I'm missing something again but, to me it looks like two examples you wrote are inconsistent:

In first example frame is passed and that's exactly an API I'm supporting. In the second example though two arguments are passed first instance to which listener was assigned and the second is port.

I also think that in the second example it's useless to pass pageMod since it's accessible from listener anyway.

Comment 6

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

(In reply to comment #5)
> Sorry maybe I'm missing something again but, to me it looks like two examples
> you wrote are inconsistent:
> 
> In first example frame is passed and that's exactly an API I'm supporting. In
> the second example though two arguments are passed first instance to which
> listener was assigned and the second is port.

I'm not sure what you mean by "API I'm supporting", but this bug is about the way event listeners are called across all the modules of the SDK, not the way they are called due to any recent work you have done with EventEmitter.

The two examples are consistent regarding the suggestion I am making in this bug, which is that the first argument to event listeners be the object that we have conventionally set as the |this| object when calling those listeners.  The presence or absence of additional arguments is irrelevant.


> I also think that in the second example it's useless to pass pageMod since it's
> accessible from listener anyway.

That's true in that particular example, but we can't guarantee that it will always be the case given .bind(); nor might it always be obvious how to access it.

Comment 7

[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 8

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

Here's a list of addon-kit modules and the events and event handler parameters their documentation describes (either in prose or in example code), in the format MODULE: EVENT([PARAMETER, ..] [NOTES]) [NOTES]

clipboard:        [none]
context-menu:     context(node) [in content script]
                  click(node, data) [in content script]
                  message(imgSrc/pageURL/imgSrcs [i.e. message])
notifications:    click(data)
page-mod:         attach(worker, mod), message(data)
page-worker:      message(message), error()
panel:            message(), show(), hide()
private-browsing: start(), stop()
request:          complete(response, request)
selection:        select(selection)
simple-storage:   OverQuota(simpleStorage)
tabs:             open(tab), close(), ready(tab), activate(), deactivate()
widget:           click(widget), mouseover(widget), mouseout(widget)
                  message(widget, message)
windows:          open(window), close(window)

I don't have any problem with example code that uses custom names for parameters (f.e. in context-menu).  And it's also fine that the documentation doesn't yet document all the events and parameters (since documenting them later isn't a breaking change).  It's even ok if parameter names aren't consistent, since changing them later doesn't break existing code.

But parameter positions must be consistent, since we can't change that later without breaking existing code.

To date, we've primarily discussed two approaches: emitter-first and emitter-last (where the emitter is the object to which the handler was added).

The emitter-first approach has the advantage of passing the emitter in a consistent position and the disadvantage of requiring consumers to declare the emitter's parameter even though it isn't needed in the common case.

The emitter-last approach has the advantage of not requiring consumers to declare the emitter's parameter and the disadvantage of passing the emitter in an inconsistent position wrt. both its index in the list of parameters and possibly eventually also its lastness (if we later identify the need to add more parameters to an existing event handler).

A third approach is to use named parameters by passing handlers a single object argument whose properties contain information about the event, just as constructors take a single "options" argument.

The named parameters approach sidesteps the position issue entirely, since named parameters can be specified in any order.  And it requires less typing than emitter-first in the common case, given destructuring assignment, although more typing than emitter-last:

     emitter-first: function (thisObject, param) { ... }
      emitter-last: function (param) { ... }
  named parameters: function ({param}) { ... }

In the uncommon case, in which the emitter is needed, the named parameters approach requires the most typing, but not by much:

     emitter-first: function (thisObject, param) { ... }
      emitter-last: function (param, thisObject) { ... }
  named parameters: function ({thisObject, param}) { ... }
                    function ({param, thisObject}) { ... }

(Typing requirements have featured heavily in some API discussions, but it's important to note that the amount of typing required is only one of a number of factors that account for the usability of an interface, and it should be treated as informative but not determinative.)

The named parameters approach is also the most similar of the three to events in the DOM (including MessageEvents emitted by window.postMessage), where handlers have a single "event" parameter whose argument is an object with properties that contain information about the event, so it is likely to be more familiar to web developers.

However, it is different from events in Google Chrome Extensions (except for a certain kind of message passing) and Node (which are designed for JavaScript engines that don't support destructuring assignment).

And it has the disadvantage vis-a-vis the other two approaches that renaming a parameter is a breaking API change, since parameters are identified by their names rather than their position.

Overall, weighing the various pros and cons of the three approaches, I am inclined to switch to named parameters across all addon-kit modules, giving handlers a single "event" parameter whose argument is an object that contains one or more named parameters, including at least a reference to the emitter (except for events dispatched in content scripts).

Furthermore, I think we should use a consistent name "emitter" (DOM equivalent: currentTarget) for the emitter parameter, so developers don't have to remember (or compute) multiple object->name associations when writing handlers.  We should continue to set the "this" object to the emitter in event handlers, however, which is both consistent with the DOM and obviates the need to access "emitter" except in functions bound to other objects via Function.prototype.bind().

(Note: DOM specifications describe events as being dispatched, whereas SDK internals describe them as being emitted, hence the name "emitter".  Neither "dispatcher" nor "emitter" seems ideal for user-facing documentation, but I don't know of a better term that is as descriptive, and "emitter" is the shorter of the two.)

And we should change two parameters' names, renaming the "message" parameter of message events to "data", which is consistent with its name in MessageEvent; and the "worker" parameter of page-mod attach events to "port", which is consistent with the naming of the similar MessagePort object in HTML5.

Implementing these changes would result in the following documented API:

clipboard:        [none]
context-menu:     context({ node }) [in content script]
                  click({ node data }) [in content script]
                  message({ data emitter })
notifications:    click({ data emitter })
page-mod:         attach({ port emitter }), message({ data emitter })
page-worker:      message({ data emitter }), error()
panel:            message(), show(), hide()
private-browsing: start(), stop()
request:          complete({ response emitter })
selection:        select({ emitter })
simple-storage:   OverQuota({ emitter })
tabs:             open({ emitter }), close(), ready({ emitter }), activate()
                  deactivate()
widget:           click({ emitter }), mouseover({ emitter })
                  mouseout({ emitter }), message({ data emitter })
windows:          open({ emitter }), close({ emitter })

(Note: I don't propose expanding documentation at this point, just changing the implementation and updating the existing documentation, as those are the only breaking changes.)

I rue not having addressed this sooner.  Now is the wrong time to be dealing with this.  Unfortunately, there isn't a better time to which we can travel to make such changes, as it'll only be worse in the future, after we have released the beta.  Thus this is the best time there is.

Comment 9

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

If we're heating that way I would also like to propose a name change from `EventEmitter` to `EventDispatcher`, since most of the node folks and their libraries will expect different behavior that will cause problems and will lead to frustrations.

Also IMO it was better to stick to a current approach since it was compatible with node, bespin, cloud9 using / switching to the same `EventEmitter` interface, besides emitter as an argument is only supposed to be used when reusing same function and even in such case you can access it from `this` pseudo variable.

Comment 10

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

(In reply to comment #9)
> If we're heating that way I would also like to propose a name change from
> `EventEmitter` to `EventDispatcher`, since most of the node folks and their
> libraries will expect different behavior that will cause problems and will lead
> to frustrations.

I see no incompatibility with Node, whose EventEmitter class emits events with arbitrary arguments.  Our implementation can continue to do the same, as we are simply establishing the convention that a high-level API event emitter will emit an event with a named parameter set as its sole argument.


> Also IMO it was better to stick to a current approach since it was compatible
> with node, bespin, cloud9 using / switching to the same `EventEmitter`
> interface

I don't know about bespin and cloud9, but the description of Node's EventEmitter interface <http://nodejs.org/docs/v0.3.1/api/all.html#events.EventEmitter> suggests that the convention we are establishing is completely compatible with it.  Where is the incompatibility?


> besides emitter as an argument is only supposed to be used when
> reusing same function and even in such case you can access it from `this`
> pseudo variable.

Sure, except when the listener is bound to another object via Function.prototype.bind().

Besides not seeing the incompatibility, I'm also puzzled by your change of heart, as you argued in favor of "event" parameters for Message events and against setting `this` for listeners in the past.  What has changed?

Comment 11

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

(In reply to comment #10)
> I see no incompatibility with Node, whose EventEmitter class emits events with
> arbitrary arguments.

The exception is the "error" event, where Node's clear convention is for the first parameter to be an exception object.  Hrmph.

I suppose a fourth approach would be to use positional parameters instead of named parameters, not include the emitter in the list of parameters, and require callers to either use |this| or close around a reference to the emitter in order to access it.

Comment 12

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

(In reply to comment #10)
> (In reply to comment #9)
> > If we're heating that way I would also like to propose a name change from
> > `EventEmitter` to `EventDispatcher`, since most of the node folks and their
> > libraries will expect different behavior that will cause problems and will lead
> > to frustrations.
> 
> I see no incompatibility with Node, whose EventEmitter class emits events with
> arbitrary arguments.  Our implementation can continue to do the same, as we are
> simply establishing the convention that a high-level API event emitter will
> emit an event with a named parameter set as its sole argument.
>
> 
> > Also IMO it was better to stick to a current approach since it was compatible
> > with node, bespin, cloud9 using / switching to the same `EventEmitter`
> > interface
> 
> I don't know about bespin and cloud9, but the description of Node's
> EventEmitter interface
> <http://nodejs.org/docs/v0.3.1/api/all.html#events.EventEmitter> suggests that
> the convention we are establishing is completely compatible with it.  Where is
> the incompatibility?
> 

You are right, but I am afraid that this can be misleading for the folks coming from the mentioned communities. I also think that for cross-platform library developers (those are most likely ones we can engage from other communities) this convention will be a pain since they will have to choose between two different conventions for their libraries to be first class citizens on either one platform or another. Also destructing assignments are only present on spider-monkey so it's less likely they will choose to stick to our convention.  

> 
> > besides emitter as an argument is only supposed to be used when
> > reusing same function and even in such case you can access it from `this`
> > pseudo variable.
> 
> Sure, except when the listener is bound to another object via
> Function.prototype.bind().
> 

Also with bind you can do emitter.on('event', listener.bind(that, emitte))

Which will make emitter first argument.

> Besides not seeing the incompatibility, I'm also puzzled by your change of
> heart, as you argued in favor of "event" parameters for Message events and
> against setting `this` for listeners in the past.  What has changed?

Comment 13

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

Attachment: pull request #74v1: implements fourth approach

Irakli and I discussed this further in IRC today, and we eventually came to the conclusion that it isn't actually necessary to pass the emitter to an event listener under any circumstance.

In the common case, a listener can access the emitter via `this`:

    let emitter = new Emitter();
    emitter.on("event", function() {
      // this == emitter
    });

In a less common case, if the listener is bound to another object, it can still often access the emitter via a closure:

    let emitter = new Emitter();
    let thing = new Thing();
    thing.listener = 
      (function() { /* emitter accessible via closure */ }).bind(thing);
    emitter.on("event", thing.listener);

And in the least common case, when the listener is bound to another object and you want to use it to handle events on multiple emitters, you can pass the emitter as an argument to the listener:

    let emitter1 = new Emitter();
    let emitter2 = new Emitter();
    let thing = new Thing();
    thing.listener = 
      (function(emitter) { /* first arg is emitter */ }).bind(thing);
    emitter1.on("event", thing.listener.bind(null, emitter1));
    emitter2.on("event", thing.listener.bind(null, emitter2));

Thus, since passing the emitter isn't necessary, it seems like the fourth option would actually be the best one, since it simplifies the API (fewer parameters, no need to destructure an event object), avoids the position issue, and is more similar to other CommonJS-implementing JavaScript environments like Node (although less similar to DOM).

I had a branch to finish implementing the third approach, but I've abandoned it now in favor of a branch to implement the fourth approach, which includes reversions of some of the changes already made (although I did most of these reversions by hand so as not to revert any ride-along changes) as well as a new set of changes for the rest of the high-level APIs.

I'm really sorry to make yet another abrupt change and undo some good work.  I wish this had all become clearer earlier.  Nevertheless, the discussion and implementation changes to date have been tremendously helpful in getting to what I think is ultimately the right approach.

Comment 14

[Drew Willcoxon :adw]

Comment on attachment 495830 [details]
pull request #74v1: implements fourth approach

Looks good.  There were one or two small problems that need to be fixed that I noted inline along with some minor nits.  So r=adw with those problems fixed.

Comment 15

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

(In reply to comment #14)
> Looks good.  There were one or two small problems that need to be fixed that I
> noted inline along with some minor nits.  So r=adw with those problems fixed.

Nits fixed as additional commits.

Branch commits:

https://github.com/mozilla/addon-sdk/commit/bc70abfa730f927621752475a8420a5547e5f4a0
https://github.com/mozilla/addon-sdk/commit/b9b57423d33c8d2d405320e9163edf6bd3b910a9
https://github.com/mozilla/addon-sdk/commit/2b7ca0873bba9a9f15bf0ebee04333a5339cbf97
https://github.com/mozilla/addon-sdk/commit/b6e6a04fadbbf6bacd62b9c056a4800da292bbdb
https://github.com/mozilla/addon-sdk/commit/632c2eedee378b4cd3134f7036d590179e94fd62
https://github.com/mozilla/addon-sdk/commit/7950b2f17a28aafecf047e22e8a424e1a5113866
https://github.com/mozilla/addon-sdk/commit/b541d0b87f247924a2e6c375500a2482be5e3366
https://github.com/mozilla/addon-sdk/commit/976f5a2ee44e3a0dd52591fdaebceb16a69d27f5
https://github.com/mozilla/addon-sdk/commit/1894027d310eae2974557ce5f4195ca40ae3c50d
https://github.com/mozilla/addon-sdk/commit/0075b34768404ea70100cafbbdb842bc9b735b57
https://github.com/mozilla/addon-sdk/commit/894a559724f5fba06597935b0fead8f902bec46d
https://github.com/mozilla/addon-sdk/commit/855831c4b498737f3b007e48281c9dad708eae54
https://github.com/mozilla/addon-sdk/commit/e4c165985dbf95092cd165efb4ebf7da8f7a5fa8
https://github.com/mozilla/addon-sdk/commit/9e4a2f50fe25e860223b0fd2dc8af76b8ddada24
https://github.com/mozilla/addon-sdk/commit/4a96c48af213a034301ed74ed03ac2c44afdae8f
https://github.com/mozilla/addon-sdk/commit/090c7cf90526fa1418d09d43d5c64440d816150c

Merge commit:

https://github.com/mozilla/addon-sdk/commit/3650333be81a5a59bd9c7a7415a3e8e96264f7ab

Comment 16

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

https://github.com/mozilla/addon-sdk/commit/7db9da62fa140e1e795bcad361d159d745a4c9a3
https://github.com/mozilla/addon-sdk/commit/f2316c687cb69c5cfee0ab9338a015383a89e76f
https://github.com/mozilla/addon-sdk/commit/7733d816d450bbee9b487a2729305c10a70a77cc
https://github.com/mozilla/addon-sdk/commit/2b1a087c6a9df8c223d4c5b76d6ebe707e958fdf
https://github.com/mozilla/addon-sdk/commit/365d0426f92108d0f60d02050812b99be6027ccf
https://github.com/mozilla/addon-sdk/commit/c0a94ba9b21bf4d3d5e0fac339e05a8ebfb878f5
https://github.com/mozilla/addon-sdk/commit/d34f0e7a70b2c65fbd114907a6d49296b1cf457d
https://github.com/mozilla/addon-sdk/commit/b035d9a2fd6bfed38a54b5bf54c7326ae2b92f47
https://github.com/mozilla/addon-sdk/commit/aa79df57140df7dec20f8f93ff8c13253afde24d
https://github.com/mozilla/addon-sdk/commit/70a6a22e891a7a3a5a50f1ead9dc7cc4e79eefc3
https://github.com/mozilla/addon-sdk/commit/d78bb0021d13142b0d1665dfe92525a3e708bb96
https://github.com/mozilla/addon-sdk/commit/6e1e32af90015610624b0749feff89829917ea14
https://github.com/mozilla/addon-sdk/commit/8ecbc8faa45493ee7e3a45c061375808fef4a416
https://github.com/mozilla/addon-sdk/commit/8b426db24cd30f62bab7a2de314507953bf5988a
https://github.com/mozilla/addon-sdk/commit/21bdeba8d0fcbb52c160b9b6df6f3b55f27d5e85
https://github.com/mozilla/addon-sdk/commit/f8024820136c41c63c8fc50f7420d909a57c4f28