slightlyoff / Promises

DOM Promises IDL/polyfill
Apache License 2.0
154 stars 28 forks source link

Remove EventedFuture #35

Closed mounirlamouri closed 11 years ago

mounirlamouri commented 11 years ago

Future no longer inherits from EventTarget but EventedFuture still does. It is not clear what is the use case of that interface.

Using .then() for Future seems to be preferred over DOM events. For example, it looks like node moved from an event based system to something else [1]. To be fair, their event based system wasn't allowing .then() but the point is that we can deduct that developers do not like event based system for promises.

Also, it is not clear what would DOM events add to the Future interface. With events, the developers would be allowed to do:

// First:
asyncCall().onaccepted = function() {
};
// Second:
asyncCall().onrejected = function() {
};
// Third:
var future = asyncCall();
async.onaccepted = function() {
};
async.onrejected = function() {
};

Which can easily be expressed that way:

// First:
asyncCall().then(function() {
});
// Second:
asyncCall().catch(function() {
});
// Third:
asyncCall.then(function() {
}, function() {
});

Using DOM events doesn't make the code easier to understand or to write. It actually removes the ability to chain calls and introduce a terrible confusion because .onaccepted and .then() do not actually behave the same way.

If you do:

var future = asyncCall();
spendTimeDoingStuffAndTripsToTheEventLoop();
// The future has been accepted.
future.onaccepted = doSomethingWhenAccepted();

and:

var future = asyncCall();
spendTimeDoingStuffAndTripsToTheEventLoop();
// The future has been accepted.
future.then(doSomethingWhenAccepted);

In the first snippet, doSomethingWhenAccepted() will never be called but in the second snippet it will.

I think it would be better to prevent that kind of inconsistency and not have this EventedFuture interface.

[1] http://howtonode.org/promises

CC @annevk @sicking @slightlyoff

slightlyoff commented 11 years ago

Punning .then() with .onaccepted is the wrong way to think about it. It's like adding a .done() handler before initial dispatch. So there isn't actually much inconsistency.

sicking commented 11 years ago

The proposal isn't to put .then() together with .onaccepted. The proposal is to remove .onaccepted entirely.

Having DOMEvents for accepted/rejected has two downsides:

  1. It means that "there's more than one way to do it". I.e. it introduces a second redundant way of getting notified when a promise is resolved.
  2. The DOMEvent way of listening for an event getting fulfilled introduces a timing hazard. The caller has to either have prior knowledge that the code has never returned to the event loop between when the Future was created and when the event-listener callback is registered, or has to remember to check the .status property to make sure that the promise hasn't been fulfilled already.

In other words, it increases the risk that some code has timing issues if it returns to the event loop between creating the Future and the callback is registered.

It's still unclear to me if having a Future which inherits EventTarget is a good idea or not. It seems like it could be useful in cases when other milestones are reached before the Future is resolved. For example in the case of indexedDB.open we need to fire a "upgradeneeded" callback sometimes before the "Future" returned from .open is resolved. And in the case of a HTTP request API it could be good to have a callback for when header data is received but before the response body has been processed.

There aren't yet any infrastructure around DOMEvents that I know of which makes it advantageous to have the callback available as an DOMEvent. SMIL has some ability to trigger animations off of arbitrary DOMEvents, but that is only used for objects that are part of the Node tree, so isn't really usable here.

slightlyoff commented 11 years ago

I understand what the proposal is, I was rejecting the argument offered for it.

As for "having more than one way to do it"...well...I'm not convinced...this is DOM and JS. There always seem multiple ways to do things around here: the shitty legacy way and the new way (which may be the future's shitty legacy).

The timing issue here is isomorphic to the timing for DOMRequest. I don't like it, and I think the control-flow inversions aren't great either...but I don't understand why there's such strong objection to the base class when DOMFuture is there primarialy for the benefit of subclasses that want to extend it.

mounirlamouri commented 11 years ago

I think it is better when there is only one way to do something, as the Zen concept of Python recommends [1]. I understand that it is often not the case with the Web Platform because there is often the legacy way and the new way but I do not understand why we should push a new feature with a completely artificial legacy API.

Also, I believe that few if not no API should require an EventedFuture. EventedFuture is worse in various ways (as described above) and specifying it in the main specification might give a wrong signal. I believe that a solution could be to add a note saying that it is possible to inherit from a Future and EventTarget if a subclass really needs DOM events but that is a pattern that isn't recommended.

[1] http://www.python.org/dev/peps/pep-0020/

sicking commented 11 years ago

Well, the fact that DOM and JS are so different is something we should try to fix, no? Or at least try to reduce their differences.

I definitely agree that the timing issues are the same ones as with DOMRequest and IDBRequest. However I'm not a big fan of how DOMRequest/IDBRequest worked out, especially when compared to promises.

The main reason that DOMRequest/IDBRequest looks the way it does is that the DOMEvents API is very constrained. It simply doesn't allow calling an individual callback that was registered after the event is fired, without also calling all other registered callbacks at the same time. Nor does it permit taking any actions when a callback is registered. I.e. addEventListener is supposed to have no side effects.

All these constraints that DOMEvents has is why I'm pushing for using them less and instead find better ways of doing callbacks. Such as Futures :)

slightlyoff commented 11 years ago

Ok, I'm removing EventedFuture. Events are the anti-pattern and I'll leave it to y'all to have a discussion with TC39 about how this doesn't step on their toes.

/cc @wycats @erights @domenic