slightlyoff / Promises

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

Creating the Resolver/Future connection #4

Closed slightlyoff closed 11 years ago

slightlyoff commented 11 years ago

The current text implies that the Future and Resolver are conencted and that only the Resolver that creates a Future can eventually resolve it. To set up this linkage, we need some way to explain how that linkage is created without magic or ES6 @-names, since they're delayed to ES7 at the soonest.

One thought is that the Resolver might expose some callback-registration methods or even events. The Future will be passed the Resolver in it's ctor and automatically connect itself to the Resolver at creation time.

Thoughts? Preferences?

/cc @domenic @wycats @arv

domenic commented 11 years ago

I assume the conflict here comes from wanting a constructor for the future at all? Otherwise there seems to be no issue with only allowing futures to be retrieved from internal APIs, or from creating a resolver first then using its future property. That is how most existing promise libraries work, i.e. you can only ever create deferreds, never promises directly (except via utility methods on the library that internally leverage deferreds).

But yeah, if the desire is to have a Future constructor, then I think something like Microsoft's approach could work:

var future = new Future(function (resolver) {
    setTimeout(resolver.accept.bind(resolver), 100);
});

This allows you to turn the tables and make resolvers non-constructible, which is a bit nice since it's a kind of low-level concept anyway.

If it weren't for the second argument to onaccept/onreject, you could then even eliminate the future property of the Resolver interface entirely, making it just a bag of methods. But I imagine things aren't going to go that far.

jakearchibald commented 11 years ago

I don't think there's a problem having both Future and Resolver constructors exposed, allows them to be subclassed. The future property on Resolver could be overwritten with a subclassed Future, perhaps with a progress method/event.

I like the MS system, although I means having a callback for what should be a sync operation (would it be sync?).

They could be tied together with events:

class Resolver {
    constructor() {
        this.future = new Future(this);
    }
}

class Future {
    constructor(resolver) {
        resolver.on('accept', this._onAccept.bind(this));
    }
}
slightlyoff commented 11 years ago

Related to this, over in #5, I'm wondering if we shouldn't allow this:

class Resolver {
    constructor(FutureType = Future) {
        this.future = new FutureType(this);
    }
}

class Future {
    constructor(resolver) {
        resolver.on('accept', this._onAccept.bind(this));
    }
}

Thereby allowing the generic Resolver class to vend multiple Future types. Thoughts? It seems like it's either that or we whack a futureType onto the Resolver prototype to enable subclasses to configure without ctor munging.

arv commented 11 years ago

FutureType makes a lot of sense to me. I think that overriding the future property is also a good alternative. Maybe it is better because it reduces the API surface area?

I really don't like the callback param to the constructor as suggested in Domenic's comment.

domenic commented 11 years ago

FWIW, MarkM has moved toward the factory function approach and is deprecating the deferred (what you call "resolver") approach:

http://wiki.ecmascript.org/doku.php?id=strawman:concurrency#q.defer

This was apparently spurred by talk with Luke Hoban at a TC39 meeting a month or two ago.

slightlyoff commented 11 years ago

Adding @lukehoban here (hi Luke!).

I can understand why this particular inversion looks good in some lights, but I'm not sold. To be fair, I don't really like either what I had above or this callback style. The class-passing style with a reified Resovler instance makes it more straightforward to write code that reads the way you mean. The callback style (sort of) eliminates a class in the system (although at this point you're using the closure of the callback as that storage) and might make it simpler not to leak the capabilities via inadvertant return of the Resolver...but that's all speculation.

They're isomorphic, and I'd prefer a version of the above that doesn't need the magical this._onAccept to envision the resolution and create the connection. Perhaps a synthesis is possible by making accept and reject instance properties and not class methods:

class Resolver {
    constructor(FutureType=Future) {
        this.accept = (function(value) { /* ... */ }.bind(this));
        this.reject = (function(error) { /* ... */ }.bind(this));
        this.future = new FutureType(this.accept, this.reject);
    }
}

class Future {
    constructor(accept, reject) {
        // ...
    }
}

In this story, it's possible to use a CancelableFuture without changing Resolver:

class CancelableFuture extends Future {
    constructor(accept, reject) {
        this.cancel = function() { reject(new Error("Cancel")); };
    }
}

var doItAsync = function() {
   var r = new Resolver(CancelableFuture);
   return r.future;
};

doItAsync.cancel(); // Our future

This can be wrapped up in the callback style API without any difficulty:

Q.promise = function(f) {
  var r = new Resolver();
  f(r.accept, r.reject);
  return r.future;
};

I accept that this doesn't work quite as well for capabilities beyond resolve and accept (e.g. progress), but adding progress was always going to require an extension of the contract on the resolver side. In the callback version, you have to now have a version of the Q function that will generate progress-savvy Promise objects. Six of one...

Anyhow, at this point the only arguments that seem to be undecided in my mind are about usability. Is it easier to teach async function authors to do the closure munging or to never return a naked Resolver object?

I honestly don't know.

Thoughts?

domenic commented 11 years ago

Well, I'm willing to be proven wrong, but I don't think it's possible to create a promise implementation that works without private state of some kind: either private state hidden in a closure, as in the instance-method version you gave and as Q does, or private state "hidden" with underscored properties, as in rsvp.js, or private state put in a private symbol as in my example, or in a weak map as we are contemplating for Q version 2.0.

Otherwise I agree with the fact that it's all tradeoffs and nothing's perfect. Unlike @lukehoban and MarkM, I'm not a huge fan of the factory function style, but on the balance I think it's a winner.

Is it easier to teach async function authors to do the closure munging or to never return a naked Resolver object?

Although I'm not sure about the former, I do know the latter is pretty easy to teach, as long as there is no then on the resolver/deferred. In Q, nobody returns deferreds; it would be annoying to do doItAsync().promise.then(...). In jQuery code, however, people return the deferreds all the time (since there a deferred is a promise with resolve/reject/etc. methods).

It's worth considering that the closure munging looks a little bit better via arrow functions:

var future = new Future(resolver => {
    setTimeout(resolver.accept.bind(resolver), 100);
});
slightlyoff commented 11 years ago

I chatted with MarkM a bit on IM...working up an alternate version of the IDL right now so we can envision how it'll work out.

Initial thoughts: we give up on having a sane cancel() and timeout() unless we bite off MANY more positional arguments. Might be worth it, or a variant with a property bag instead of positional args.

slightlyoff commented 11 years ago

Ok, here's the callback version. I hate where it leaves callback/timeout, but the minimal sample code shows the overhead of the extra function(a, r) { ... }. It ain't pretty, but it's arguably easier to understand if you grok closures:

https://github.com/slightlyoff/DOMFuture/blob/master/DOMFuture_callback.idl

Thoughts?

domenic commented 11 years ago

The approach we're going for in Promises/A+ is to pass a resolver object to the callback, although there's debate as to how exactly it works. One idea is a resolve function which has resolve.reject, resolve.cancel, resolve.progress, and (controversially) resolve.fulfill. Another is a simple property bag, i.e. resolver.resolve, resolver.reject, ...

slightlyoff commented 11 years ago

yeah, actually, I was looking at a 3rd param for side contracts:

   var accept, reject, cancel, timeout;
   var f = new Future(function(a, r, others) {
      accept = a; reject = r;
      cancel = others.cancel;
      timeout = others.timeout;
      // ...
   });
slightlyoff commented 11 years ago

Ooooh... I think I understand. Would the property bag get filled in? E.g.:

var resolve = {};
var f = new Future(resolve);
// cancel later
setTimeout(resolve.cancel, 1);
domenic commented 11 years ago

No, although that's an interesting idea... It was more conventional:

var f = new Future(resolve => {
  if (Math.random() > 0.5) {
    resolve("yay!");
  } else {
    resolve.reject(new Error("no good!"));
  }
});
slightlyoff commented 11 years ago

I guess that's just the extreme form of the others bag. I suppose I don't really like the idea of the ctor counting on the mutability of an argument (and I know MarkM will object strenuously), and I really do want extensibility. Perhaps the property bag is the way to go.

I do loathe the above creating a generic resolution (not acceptance?) and then having rejection as a property, though. That's just fugly.

@wycats, @arv, @lukehoban...thoughts?

domenic commented 11 years ago

On resolution vs. acceptance/fulfillment... what a can of worms >_<. Might want to open a new issue if you're interested in discussing this further so that we don't derail this one. But while I'm here...

MarkM, @kriskowal, and @briancavalier are convinced that "verbatim fulfillment" is not OK, because promises-for-promises are hazardous to certain invariants or identities that otherwise hold. Long-form reasoning from Brian here. I don't entirely agree, but see their points.

This is also informing our thinking on what throw promise inside onFulfilled and onRejected should do, see problem and solution. Edge-casey, but, for a spec, important.

Finally, we're having this exact debate, of resolve vs. fulfill/accept, over in this long thread that you probably don't want to wade through. In short, a simple fulfill|accept/reject model is pedagogically and conceptually simpler, but breaks a number of invariants that some people find important.

slightlyoff commented 11 years ago

We break identity every time we .bind(). This ship has sailed. If you want identity, you need a new side contract.

Similarly, I don't actually care about exception handling. We're inverting exception control flow via reject and demanding that we now treat the synchronous case sanely without demanding that people use the provided APIs feels like it's just trying far too hard to have an opinion about something that doesn't need to matter.

@arv @wycats : am I being nuts here?

slightlyoff commented 11 years ago

I've revised the alternate IDL [1] and I'm happy with how the single-arg callback bag works, so I'm closing this and I'm going to start re-working DOM APIs in terms of this design.

If you hate this, please open a new issue = )

[1] https://github.com/slightlyoff/DOMFuture/blob/master/DOMFuture_callback.idl