Strawman: Promise Creation API/D #18

Open
domenic opened this Issue Feb 10, 2013 · 39 comments

4 participants

@domenic
Promises/A+ member

Terminology

Some additional terms are introduced for talking about promises:

  1. "Settled" means the promise's state is either fulfilled or rejected.

Promise Fates

A promise's fate describes what can happen to the promise in the future. It is a distinct concept from its state, although they are related.

A promise can have one of two fates:

  1. "Unresolved" means that a promise's fate has not yet been determined: it is still pending, and the promise resolution procedure has never been called to resolve the promise.
  2. "Resolved" means that a promise's fate has been determined: it is either settled, or pending because it has adopted the state of a pending thenable via the promise resolution procedure.

Note that a promise can only move from unresolved to resolved, never the other way around.

The Promise Constructor

An implementation of this specification supplies a function responsible for constructing new promises. This promise constructor takes as its sole argument a user-supplied resolver function which is given the means to resolve the constructed promise.

Here we refer to this promise constructor by the name Promise for concreteness, although the name is implementation-specific.

var promise = new Promise(resolver);
  1. Can be called without new, with the same results.
  2. Object.getPrototypeOf(promise) === Promise.prototype must be true.
  3. promise.constructor === Promise.prototype.constructor === Promise must be true.
  4. Promise.length === 1 must be true.
  5. If resolver is not a function, the implementation must throw a TypeError.
  6. If resolver is a function,
    1. It must be called with the promise's resolver arguments.
    2. It must be called immediately, before Promise returns.

The Resolver Arguments

When resolver is called, it is given the constructed promise's resolver arguments, which have the ability to resolve the promise.

var promise = new Promise(function (resolve, reject) {
  // ..
});

Calling resolve(x)

  1. If promise is resolved, nothing happens (in particular, no exception may be thrown).
  2. Otherwise, run the Promise Resolution Procedure [[Resolve]](promise, x).

Calling reject(reason)

Calling reject(reason) resolves the promise to a rejection reason (i.e. rejects the promise).

  1. If promise is resolved, nothing happens (in particular, no exception may be thrown).
  2. Otherwise, reject promise with reason as its rejection reason.

Resolver Throwing

If resolver throws an exception, call it e,

  1. If promise is resolved, nothing happens (in particular, no exception may be thrown).
  2. Otherwise, reject promise with e as its rejection reason.
@novemberborn

Promise.length === 1 must be true.

This precludes implementations from extending behavior, e.g. by accepting an options object as the second argument.

If factory is a function,
It may only be called when the function execution stack is empty. (note 1)

What is your reason for this? Take the following example:

function foo(){
  var bar = "baz";
  var promise = new Promise(function(resolve){ resolve(bar); });
  bar = "thud";
  return promise;
}

The result would be thud, but the analogous function using a defer() would result in baz:

function foo(){
  var bar = "baz";
  var dfd = defer();
  dfd.resolve(bar);
  bar = "thud";
  return dfd.promise;
}
@ForbesLindesay
Promises/A+ member

I don't like the fact that you can't build defer out of this spec (without creating your own proxy to the resolver which waits for the resolver to be available, and that's kind of insane).

Requiring Promise.length === 1 doesn't play nicely with our current straw-man for cancellation.

@briancavalier
Promises/A+ member

This precludes implementations from extending behavior, e.g. by accepting an options object as the second argument.

Maybe hair splitting, but technically, it doesn't. There can be additional args, they just can't be in the signature. I believe ES5 has used this in many places for optional args, e.g. Array.prototype.reduce.length === 1. Granted, that may be a different use case (optional arg, vs. extension point).

It may only be called when the function execution stack is empty. (note 1)

I'd like to understand the reasoning behind this one as well, especially given the observations of @novemberborn and @ForbesLindesay.

I assume we'd also be ignoring any return value from factory? This is what we had discussed in the other constructor-based proposals, so just want to make sure.

@novemberborn

Maybe hair splitting, but technically, it doesn't. There can be additional args, they just can't be in the signature.

Ha! At that point though, why bother specifying length at all. At least with reduce it helps differentiate between an initial undefined accumulator and none at all.

@briancavalier
Promises/A+ member

@novemberborn like I said, I was splitting hairs :) I'd also like to understand @domenic's reasons for specifically requiring length 1.

@domenic
Promises/A+ member

Promise.length === 1

I'm not married to this, but, just like in the ES5 specs, this would signal that any other params have default values. (Indeed, in ES6, a function's length is equal to its number of parameters without defaults.)

Next turn requirement

This is crucial if we want the thenable assimilation equivalence. Thenables are not to be trusted; Promise.from must be a safe way to consume untrusted thenables and turn them into trusted promises, without the hazard of interleaving untrusted code (i.e. interleaving thenable.then). See MarkM's slides, "Publish or Perish" section.

Furthermore, I think it actually works out nicely. It solves the problem of what to do if factory throws, and has no performance impact since now resolver(x) and resolver.reject(reason) can just immediately modify the state of the returned promise, since they're guaranteed to be called in the next tick.

@domenic
Promises/A+ member

I just noticed my original assimilation equivalence was broken, since I used resolver + resolver.reject instead of resolve + reject. This is unfortunate, as it means I had to amend the assimilation equivalence to

new Promise(function (resolver) { thenable.then(resolver, resolver.reject); })

I'm really torn between resolver + resolver.reject and resolve + reject. The latter is nice and fits existing practice, but is bad for extensibility.

@ForbesLindesay
Promises/A+ member

the assimilation as construction could really alleviate this problem.

@domenic
Promises/A+ member

Updated to switch back to (resolve, reject).

@briancavalier
Promises/A+ member

Just noticed that Promise.from 3 implies that thenable.then should be called as a function (without its original this). That may not be valid for some thenables. In fact, since we didn't make that a requirement of Promises/A+, a promise might fail there too. I think we probably just need to tweak the wording/example.

@domenic
Promises/A+ member

Good call, @briancavalier. We probably should just rephrase that to use more general prose. I also don't want to mandate how implementations implement the assimilation; I tried to kind of get around that by saying "the result of", but it could be done better.

@novemberborn

Next turn requirement

This is crucial if we want the thenable assimilation equivalence. Thenables are not to be trusted; Promise.from must be a safe way to consume untrusted thenables and turn them into trusted promises, without the hazard of interleaving untrusted code (i.e. interleaving thenable.then).

Sure, but the library could ensure thenable.then is called in the next turn by wrapping it; this does not require the factory method itself to execute in the next turn.

Furthermore, I think it actually works out nicely. It solves the problem of what to do if factory throws

Does it? This draft doesn't mention any behavior, is it supposed to be an uncaught exception? At least if we throw synchronously I'm likely to be constructing a promise from within another promise callback so my program won't crash.


Regarding resolve, reject or resolve.reject I could go either way. However for extensibility I'd prefer if methods are added to resolve, so why even have a free reject method?


In the following example, having a separate factory method is cumbersome compared to having a defer():

User.prototype.getEmail = function(){
  return new Promise(function(resolve))
    db.users.findOne({ _id: this.id }, { email: true }, function(err, doc){
      if(err){
        resolve.reject(err);
      }else{
        resolve(doc.email);
      }
    });
  });
};

Note that the factory needs to access this.id. I could bind the factory but that'd be slow. I could store the id or this in a variable but that becomes cumbersome. It'd be nice to either be able to provide a thisArg, or pass extra arguments into the factory. Still, that may be an extension point for implementations, and it's a problem with then() anyway.

@ForbesLindesay
Promises/A+ member

The issue with then will be fixed by ES6 arrow functions so I don't think we should worry about it at all.

@domenic
Promises/A+ member

I think this is pretty good, but we have some remaining issues to flesh out as a group. I'll post new issues with them before we can converge on something more final.

@novemberborn

Some questions in trying to implement this:

If x is a thenable, promise must attempt to adopt the state of x (see Promises/A+ spec).

Can resolve be called multiple times, with multiple thenables, effectively racing to the first thenable to fulfill?

Is the following an appropriate algorithm for adopting the state of the thenable?

function adoptState(thenable, resolve, reject){
  try{
    thenable.then(resolve, reject);
  }catch(error){
    reject(error);
  }
}

Note that if the thenable fulfills with another thenable, resolve will try to adopt that state instead.

@domenic
Promises/A+ member

@novemberborn due to promises-aplus/promises-tests#20 I realized that "adopt the state" is not very well specified, and outlined the problem in promises-aplus/promises-spec#75.

My solution is to specify "thenable assimilation" much more in-depth. The algorithm is exactly like yours; you can see it in prose over in at promises-aplus/promises-spec#76. In particular, the recursive-adoption strategy is present.

@novemberborn

I've implemented this draft in https://github.com/novemberborn/legendary/blob/3f96b54de4e76dabe8658980cc54ba502b22c339/lib/Promise.js, using same-turn/crash invocation of the factory method.

@domenic
Promises/A+ member

Updated after some discussion in #20, #21, #22.

  • Assimilation removed
  • Factory is no longer invoked asynchronously
  • Throwing an error in the factory behaves like reject.
@domenic
Promises/A+ member

Renamed "factory" to "resolver" as per @wycats. It's so obvious in hindsight :O.

@domenic domenic referenced this issue in promises-aplus/promises-spec Feb 27, 2013
Merged

Add the "Thenable Assimilation Procedure". Closes #75. #76

@ForbesLindesay
Promises/A+ member

I really dislike "The Promise Constructor - 5":

If resolver is not a function, the implementation must throw a TypeError.

Since my primary export for my library is the promise constructor, I'd really like to have it act as a clever polymorphic function capable of handling assimilation and converting a value into a promise for a value. Failing this I'll have to have an adapter that looks like:

var Promise = require('promise');

function Adapter(fn) {
  if (typeof fn !== 'funciton') throw new TypeError('fn is not a function');
  return Promise.apply(this, arguments);
}

Adapter.prototype = Object.create(Promise.prototype);
Adapter.prototype.constructor = Adapter;

module.exports = Adapter;

Which feels like cheating.

@domenic
Promises/A+ member

Since my primary export for my library is the promise constructor, I'd really like to have it act as a clever polymorphic function capable of handling assimilation and converting a value into a promise for a value.

This seems like a very bad idea. In particular, making Promise(func) behave differently from Promise(obj) is not good.

@ForbesLindesay
Promises/A+ member

Providing both return a promise, I don't think it is. You're having the later be an error state, I think it's a very useful non-error state.

@domenic
Promises/A+ member

They have entirely different semantics. Promise(obj) behaves as Promise(function (resolve) { resolve(obj); }), whereas Promise(func) does not behave as Promise(function (resolve) { resolve(func); }). Very bad.

@ForbesLindesay
Promises/A+ member

That is a fair point, I hadn't considered that case 😫

I now fully support this spec as it currently stands (I think). I'm about to have a go at implementing it for then/promise

@ForbesLindesay
Promises/A+ member

I've just partially broken anyone who doesn't use semver properly to lock down their version numbers for promise by updating it to this API. It should still run but it prints nasty deprecation warnings.

I've also written a sketchy unit test for this API at then/promise/test/resolver-tests.js. There are loads of corner cases it doesn't test, but it at least aims to test the things most likely to fail.

The only thing I know isn't inline with this API is that I currently support returning a deferred object if you call promise() without new and with a falsey resolver. It does show deprecation messages when you do that though, and I will modify it in the future, probably making defer a separate library.

@ForbesLindesay
Promises/A+ member

OK, you were write about not using Promise(obj) as a shortcut for Promise((resolve) => resolve(obj) (that would have turned out to be a really bad idea.

Is there any reason why we can't support Promise(thenable) as a shortcut for Assimilate(new Promise, thenable) as defined in the promises spec?

Point 5 becomes 2 points:

  1. If resolver is not a function or a thenable, the implementation must throw a TypeError.
  2. If resolver is a thenable, run the Thenable Assimilation Procedure Assimilate(this, thenable) (see Promises/A+ spec).

That doesn't seem like too much overhead to put on implementers and it would be a hugely useful feature.

@briancavalier
Promises/A+ member

My personal preference is for functions/methods to do one thing.

That doesn't seem like too much overhead to put on implementers and it would be a hugely useful feature

What advantages do you see over having 2 separate APIs, one for construction and one for assimilation?

It may be a point of confusion for implementors and users if we end up specifying a sync resolver callback, but async assimilation inside the same constructor.

@ForbesLindesay
Promises/A+ member

The downside is verbosity:

var promise = require('promise');

var p = promise(thenable);
var p2 = promise((resolve) => resolve(10));

vs.

var promise = require('promise');

var p = promise.assimilate(thenable);
var p2 = promise((resolve) => resolve(10));

I prefer the former, and I think assimilating a thenable is a very common thing to want to do.

@domenic
Promises/A+ member

I too think functions that do only one thing is a good idea. (Also, it helps optimizing compilers.)

@ForbesLindesay, I think you have trapped yourself in a corner by making your library's primary export be the promise constructor, and now you are trying to influence the promise constructor API to make it work better as your library's primary export.

@ForbesLindesay
Promises/A+ member

I mostly think the primary output of a library should be the constructor, much like the primary output of an Array module would have been Array and the primary output of a string module would have been String had we been using module systems when they were created. It's not a huge problem as at some point I intend to create a higher level promise library (with an API size similar to Q) on top of Promise, and that doesn't need to export the constructor as the primary export.

Thinking about the eventual road to standardization and inclusion in a core JS spec (which is what we eventually want), the core thing we'd expect to standardize would be the Promise constructor, not some wrapper around a Promise constructor, hence the point about letting it be called with or without new.

@novemberborn

I've implemented the latest version of this draft in legendary@0.8.0 and it works quite nicely. What are our next steps here?

@ForbesLindesay
Promises/A+ member

Someone needs to add this document as the readme (marked as a working draft), then we need to begin getting it ratified, just like we're currently doing the next versions of Promises/A+

@briancavalier
Promises/A+ member

when.js 2.0 also includes this API, although it is not public, and only used internally. I am planning to expose it in an upcoming release.

I agree it would be a good forcing function for us to move this to the README. How do others feel about that?

@domenic
Promises/A+ member

RSVP v2 also has this.

I think this is close to complete, but wanted to get 1.1 of the main spec all done before spending much more time on it.

@briancavalier
Promises/A+ member

@domenic I agree about getting promises-spec 1.1 done first.

@domenic
Promises/A+ member

Latest TC39 thinking is that new built-ins should not have [[Call]] do the same as [[Construct]]. For example, this won't be the case for Map, Set, etc. See http://www.esdiscuss.org/topic/map-etc-and-call; this will also be a subjection of discussion by @allenwb at the upcoming TC39 meeting.

Thus, I think we should drop the "can be called without new," to get the same results. I am not sure what exact semantics we should specify, if any; ideally I'd like to be strict here and proscribe specific ones so that nobody writes code that depends on being callable without new in a reusable library. The es-discuss thread is suggesting throwing if called without new, but this prevents subclassing.

@briancavalier
Promises/A+ member

Thanks for the pointer @domenic. Yeah, if that's the ES6 direction, I agree we should head that way as well and remove "can be called without new". Has a final decision been made by TC39 yet?

I can't really see a good answer to the strict vs. allowing subclasses issue--call and apply need to work, or a promise implementation has to provide some custom way to extend its promises. Perhaps we should just rely on people learning that they should use new, and let the chips fall where they may. We could also include language saying that to create a new promise, Promise must be called using new.

@domenic
Promises/A+ member

I think it's pretty final; the new semantics are widely agreed upon, although this particular consequence of it is just starting to come out into the open. See also slightlyoff/Promises#71.

One thing, perhaps, would be disallow special-case code like if (!(this instanceof Promise)) { return new Promise(resolver); }. This should give approximately the right semantics, since then the constructor will fail---in strict mode, at least---when it tries to do anything with this.

@ForbesLindesay
Promises/A+ member

Preventing special case code would certainly be helpful for parasitic inheritance.

@terinjokes terinjokes added a commit that referenced this issue Nov 11, 2013
@terinjokes terinjokes Repo-ification of #18 ec7d511
@npmcomponent npmcomponent pushed a commit to npmcomponent/then-promise that referenced this issue Jan 6, 2014
@ForbesLindesay ForbesLindesay Implement resolvers strawman/D 7dd7c9f
@ForbesLindesay ForbesLindesay referenced this issue in promises-aplus/promises-tests Jul 14, 2014
Closed

Calling resolve twice is not specified #62

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment