EventEmitter.once() should return a promise

RESOLVED FIXED in Firefox 29

Status

()

Firefox
Developer Tools
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jwalker, Assigned: jwalker)

Tracking

Trunk
Firefox 29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

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?
(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
(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.
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?
(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

4 years ago
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.
(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

4 years ago
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…
(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

4 years ago
(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.
Irakli, do you have thoughts on this proposal and it's compatibility with the addon-sdk?
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.
Flags: needinfo?(rFobic)
Created attachment 8359194 [details] [diff] [review]
oncepromise-927423.patch
Assignee: nobody → jwalker
Status: NEW → ASSIGNED
Attachment #8359194 - Flags: review?(bbenvie)
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").
Attachment #8359194 - Flags: review?(bbenvie) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/09a1b0df0d4c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.