slightlyoff / Promises

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

.then() on Resolver instances? #5

Closed slightlyoff closed 11 years ago

slightlyoff commented 11 years ago

One question has nagged at me related to @domenic's question regarding cancelation. It seems that some APIs will with to return resolvers to expose the ability to resolve. The natural style of use is broken for these APIs:

// Note that we must dig out the future property in order to .then()
var r = doThing();
// ...
r.future.then(function(v) { ... }, ...);

// This doesn't work:
doThing().then(...);

To repair this, should a Resolver expose a .then() method which forwards to its .future's .then()?

E.g.:

Resolver.prototype.then = function() {
   return this.future.apply(this.future, arguments);
};

If we do this, should returning a Resolver in a .then() also be treated the same way as returning a Future?

domenic commented 11 years ago

I don't like this API in general; it creates too much of a temptation to hand out resolvers. Look at how people use $.Deferred and very rarely use the .promise() method, instead referring to it as "jQuery Deferreds" and just passing out the deferred.

domenic commented 11 years ago

Whoops, meant to write the constructive paragraphs before hitting "Comment."

In general I think the right approach here is subclassing of the future to add addtional capabilities, whether they be progress or cancellation. (I think wanting to add accept/reject capabilities will be very rare.)

In particular, @kriskowal and I have been kicking around the idea of cancellable promises for Q---the idea being a promise with a cancel() method. This cannot be part of the base promise, since it creates a communication channel that allows promise consumers to affect the state of the ongoing operation. But, as an opt-in feature, it's reasonable: e.g. var p = createCancellablePromise(deferred), where, roughly,

function createCancellablePromise(deferred) {
    var p = clone(deferred.promise);
    p.cancel = deferred.cancel;
    return p;
}

APIs like an XHR wrapper could then return these promises, which are not POLA-compliant, but are useful for modeling the particular operation of XHR.

slightlyoff commented 11 years ago

I think this is good. Not adding a .then() to Resolvers.

One thing I'd like to get closure on (har har) is how difficult it's going to be to subclass since you need to subclass both the Future type and the Resolver to vend the new future type. Perhaps we should allow an argument to the Resolver constructor with the Future function object to new-up/vend? Thoughts?

domenic commented 11 years ago

Hmm. I think that can be avoided, if you go with the pattern wherein resolvers are not created independently, but instead the idea is

var future = new Future(function (resolver) {
  // use resolver here
});

In this case you'd subclass Future only. I envision it like so:

var _future = new Symbol();
var _reject = new Symbol();

class Future {
  constructor(fn) {
    var resolver = new Resolver(this);
    fn(resolver);
  }

  [_reject](reason) {
    // modify internal state; this is only called by the resolver
  }
}

class Resolver {
  constructor(future) {
    this[_future] = future;
  }
  reject(value) {
    this[_future][_reject](value);
  }
}

class CancellableFuture {
  cancel() {
    this.reject(new CancellationError());
  }
}

Make sense, or am I missing the mark?

slightlyoff commented 11 years ago

Hm. That sort of inverts the way I'd been imagining things so far, but perhaps it's workable. In my current version, Future instances are created by Resolvers. I.e., if you new up a Resolver, a Future for it is created:

function asyncWork() {
    var r = new Resolver();
    // ...
    return r.future; // Auto-created Future
}

It seems like your inversion of this does adequately prevent Resolver instances from leaking, which is good, however I'd like a variant that doesn't rely on ES6 (since Symbol implicates not only symbols and likely strict mode but also the module system); I think this might be why I hadn't gone this route before, now that I think back on it.

domenic commented 11 years ago

Well, it is the DOM, after all, so private state via a variety of mechanisms is available. E.g. weak maps or whatever browsers are using internally. Even closures, if you want to stick with pure-ES5-no-DOM semantics.

slightlyoff commented 11 years ago

An explicit goal of this spec is to avoid magic such that whatever eventual API we design can be imported (wholesale or subsetted) to ES7, so leaning on unexplained DOM magic is a no-go here.

I think closure desugaring is probably where we want to be for the minute, although I'm not sure it entirely resolves the usability questions: Is it actually friendly to require folks to dig the resolver out of the ctor's callback param? I'm not strictly opposed, but I'd like to see what others think.

@wycats? @dglazkov? @arv?

annevk commented 11 years ago

Why does this need to be imported to ES7? JavaScript as a language has no concept of asynchronicity. (At least as far as I can tell all that is defined by HTML.)

arv commented 11 years ago

@annevk ES6 will have to spec this anyway for the module loaders. For ES7 we have to spec end of microtask for Object.observe (or refer to the DOM spec if we are happy with such dependencies).

slightlyoff commented 11 years ago

What @arv said. ES will have an observable task queue via Object.observe (which in browsers will be the same as we use for Mutation Observers). In any case, ES needs this feature as much (if not more) than DOM does. DOM can move first, so that's why we're working on it here.

slightlyoff commented 11 years ago

I think this is now resolved in #4. I'll remove the old file and move the new one to its place, closing this issue when I do.