927423 - EventEmitter.once() should return a promise

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Joe Walker [:jwalker]]

once: function EventEmitter_once(aEvent, aListener) {
    let deferred = promise.defer();

    let handler = function(ev) {
      this.off(aEvent, handler);
      aListener.apply(null, arguments);
      deferred.resolve(ev);
    }.bind(this);

    handler._originalListener = aListener;
    this.on(aEvent, handler);

    return deferred.promise;
  },

Or something. Are there any events that have more than one parameter?

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Joe Walker [:jwalker] from comment #0)
> Are there any events that have more than one parameter?

Yes, things like: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/observable-object.js#l89

Comment 2

[Joe Walker [:jwalker]]

(In reply to J. Ryan Stinnett [:jryans] from comment #1)
> (In reply to Joe Walker [:jwalker] from comment #0)
> > Are there any events that have more than one parameter?
> 
> Yes, things like:
> http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/
> observable-object.js#l89

Gak. That's annoying.

So we have 3 options that I can think of:

1. once() returns a promise that resolves to the first parameter to the event listener, the other parameters are ignored
2. once() returns a promise that resolves to an array containing the full list of arguments
3. once() returns a promise that resolves to the first parameter if there is only one, or an array if there are several

Which results in something that is:

1. nice 99.9% of the time, but for the remaining 0.1%
2. annoying 99.9% of the time, but usable 100%
3. nice 100% of the time, but slightly voodoo and unexpected

I'm torn between 1 and 3.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

If this behavior is not widespread, can't we just always emit a single event parameter, just like DOM's CustomEvent API does with the |detail| property?

Comment 4

[Joe Walker [:jwalker]]

(In reply to Panos Astithas [:past] from comment #3)
> If this behavior is not widespread, can't we just always emit a single event
> parameter, just like DOM's CustomEvent API does with the |detail| property?

Would that mean altering the first parameter to add a detail property? I'm not sure how that works.

Comment 5

[Dave Camp (:dcamp)]

Note that addon-sdk has an event target api that's very similar to ours;  differences from that API should have a Darned Good Reason, I think.

Comment 6

[Joe Walker [:jwalker]]

(In reply to Dave Camp (:dcamp) from comment #5)
> Note that addon-sdk has an event target api that's very similar to ours; 
> differences from that API should have a Darned Good Reason, I think.

Agreed, although this is a strictly backwards compatible extension. It makes the addon-sdk EventEmmitter a pure subset of our EventEmmitter, so I think it just need a Fairly Good Reason - which in my book is: The clash between promises and events is hard, we should do anything we can to smooth this over. Proof would be in the many tests that this helps. e.g. [1]

[1]: https://hg.mozilla.org/integration/fx-team/file/ef27681c50f7/browser/devtools/debugger/test/head.js#l189

Comment 7

[Paul Rouget [:paul]]

Why do we want to do that? (EventEmitter.once() should return a promise)

Why not create a new function name?

> EventEmitter.onceAsPromise()

or something similar…

Comment 8

[Joe Walker [:jwalker]]

(In reply to Paul Rouget [:paul] from comment #7)
> Why do we want to do that? (EventEmitter.once() should return a promise)

In a test that uses Task.jsm:

    thing.openAsynchronously();
    yield thing.once('open');
    thing.doSomethingWhenOpened();

This hugely simplifies writing tests.

> Why not create a new function name?
> 
> > EventEmitter.onceAsPromise()
> 
> or something similar…

Only because we don't need to. I didn't see much benefit in having 2 functions, but I'm not hugely fussed.

Comment 9

[Dave Camp (:dcamp)]

(In reply to Joe Walker [:jwalker] from comment #6)
> (In reply to Dave Camp (:dcamp) from comment #5)
> > Note that addon-sdk has an event target api that's very similar to ours; 
> > differences from that API should have a Darned Good Reason, I think.
> 
> Agreed, although this is a strictly backwards compatible extension. It makes
> the addon-sdk EventEmmitter a pure subset of our EventEmmitter, so I think
> it just need a Fairly Good Reason - which in my book is: The clash between
> promises and events is hard, we should do anything we can to smooth this
> over. Proof would be in the many tests that this helps. e.g. [1]

Good extension (although we'd like to converge on implementations, so maybe discuss this change with irakli?).

I actually meant that changing the style we recommend using (single-arg events with detail properties) puts us at odds with the suggested way to use a very similar API.

Comment 10

[Joe Walker [:jwalker]]

Irakli, do you have thoughts on this proposal and it's compatibility with the addon-sdk?

Comment 11

[Joe Walker [:jwalker]]

Irakli, I'm minded to fix this. We are currently writing worse tests because this is missing.

In reference to the debate about parameters above, I intend to return a promise that resolves to the first parameter only (i.e. option 1). Anyone wanting to use the once with a multi-parameter event would need to continue to use a callback.

Please shout soon if you have an opinion.

Comment 12

[Joe Walker [:jwalker]]

Attachment: oncepromise-927423.patch

Comment 13

[Brandon Benvie [:benvie]]

Comment on attachment 8359194 [details] [diff] [review]
oncepromise-927423.patch

Review of attachment 8359194 [details] [diff] [review]:
-----------------------------------------------------------------

A small concern I have is adding extra overhead to `once`, but it's probably not an issue since it's usually a *ahem* once off usage that doesn't affect performance.

::: browser/devtools/shared/event-emitter.js
@@ +72,5 @@
>        this.off(aEvent, handler);
> +      if (aListener) {
> +        aListener.apply(null, arguments);
> +      }
> +      deferred.resolve(arguments[1]);

It would be better to name the argument to make it obvious what we're doing here (something like "firstArg").

Comment 14

[Joe Walker [:jwalker]]

(In reply to Brandon Benvie [:benvie] from comment #13)
> It would be better to name the argument to make it obvious what we're doing
> here (something like "firstArg").

Will do.

While I'm here: Green on try: https://tbpl.mozilla.org/?tree=Try&rev=20eaebe0f458

Comment 15

[Joe Walker [:jwalker]]

Landed.

https://hg.mozilla.org/integration/fx-team/rev/09a1b0df0d4c
https://tbpl.mozilla.org/?tree=Fx-Team&rev=09a1b0df0d4c

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/09a1b0df0d4c