promises-aplus / constructor-spec

Discussion and drafts of a possible spec for creating and resolving promises
10 stars 4 forks source link

promise(function (resolver)) or promise(function (fulfill, reject, progress, cancellationToken, ...)) #7

Open ForbesLindesay opened 11 years ago

ForbesLindesay commented 11 years ago

If we go down the route of #3 then we need to decide what arguments get passed to the function. If people end up going different ways on this we'd have no interoperability, and this wouldn't be easy to change in a backwards compatible manner.

Do people prefer:

return new Promise(function (resolver) {
    resolver.fullfill('foo');
  });

(where resolver has properties for fulfill, reject, reportProgress, cancellationToken etc.)

or

return new Promise(function(fullfill, reject, progress, cancellationToken){
    fullfill('foo');
  });

(the order of these arguments is arbitrary now, but could never be changed once decided)

or

return new Promise(function (fullfill, reject, options) {
    fullfill('foo');
  });

(where options has properties for reportProgress, cancellationToken etc.)

ForbesLindesay commented 11 years ago

Personally I'm fairly happy with option 1 or option 3, but I think option 2 would be a bad idea (frustrating to implement in libraries that don't yet support everything and gets worse and worse if we change/add things).

I think I marginally prefer option 1 as I think it remains helpfully consistent with other APIs such as spawn, which might end up looking a bit like this:

spawn(function* (resolver) {
  var a = yield promiseA;
  resolver.reportProgress(0.5);
  var b = yield promiseB;
  resolver.reportProgress(1);
  return a + b;
});
juandopazo commented 11 years ago

I prefer Library.defer(function (fulfill, reject) {}) only if the number of parameters is never greater than 3.

If it needs to be greater than 3, some implementations would add more than 3 regardless of the standard (extending it) or we believe it may need more than 3 at some point in the future, then I prefer Library.defer(function (resolver) {}).

ForbesLindesay commented 11 years ago

There are currently these 4 things that we know we'll need to pass to the function based on the current specs in development. I believe Q might have some other wacky way to semi-resolve a promise @kriskowal ? I'm planning to make a library that supports hot/cold promises that start only when you call then.

So this number is going to increase, but we could just separate out fulfill and reject.

Another alternative:

return new Promise(function (resolver, fullfill, reject) {
    fullfill('foo');
    //or
    resolver.fulfill('foo');
});

(where resolver has properties for fulfill, reject, reportProgress, cancellationToken etc.)

That way if you just need fulfill, you could do something like:

return new Promise(function (_, fullfill) {
    fullfill('foo');
});
domenic commented 11 years ago

I am fairly strongly in favor of option 1, with resolver alone. Anything else seems needlessly messy.

ForbesLindesay commented 11 years ago

I'm weakly in favour of option 1, I also think we should only specify that first argument, so implementers can put extensions into the second argument.

kriskowal commented 11 years ago

I think we’re least likely to paint ourselves into a corner with option 1.

briancavalier commented 11 years ago

+1 for option 1

briancavalier commented 11 years ago

One potentially interesting observation is that with option 1, it's possible to have an identical resolver API for both a Promise(function(resolver){...}) approach and a let {resolver, promise} = defer() style approach (as per #2).

novemberborn commented 11 years ago

Just now catching up on the discussion here. +1 for option 1.

kriskowal commented 11 years ago

I happened on a chance to chat with Mark Miller today. He favors:

var promise = Q.promise(function (resolve) {
    resolve(value);
})

Where "resolve" may play both role of the "resolve()" function and "resolver" object in the sense that any additional methods may be properties of that function. Thus resolve.notify(progress).

MarkM also strongly favors the ability to pass "resolve" as a free function.

domenic commented 11 years ago

Back to the dual-role resolve function... :-/. That's always been confusing for me.

To clarify, I assume that resolve(notAPromise) fulfills promise with notAPromise, and resolve(otherPromise) fulfills or rejects promise in the same way as otherPromise.

Still, as long as it has .fulfill and .reject properties that do what you expect, it's not so bad. You could even see it as a useful way of "hiding" the function currently called deferred.resolve, and which we were considering calling resolver.become.

I also think it should be spelled Q.Promise or new Q.Promise.

kriskowal commented 11 years ago

MarkM is also adamant that fulfill(promise) is a terrible idea and that we should solve the pedagogical problems with resolve(value or promise) in some other yet-imagined way.

kriskowal commented 11 years ago

To be clear, MarkM’s complaint is that fulfill causes more pedagogical problems than it solves:

var rejection = reject(new Error("Can't do it"));
fulfill(rejection).then(function (x) {
    // x === rejection
})
domenic commented 11 years ago

Wow, that example seems perfectly straightforward to me. Rename x to fulfillmentValue and I think it makes total sense.

I guess the issue is whether you are dealing with maybe-promises, or whether you know at all times whether you have a promise or a non-promise value. My experience has always been the latter. And I think most people have the same experience. Otherwise we'd all be using Q.when(unknown, ...) instead of promise.then(...).

novemberborn commented 11 years ago

Where "resolve" may play both role of the "resolve()" function and "resolver" object in the sense that any additional methods may be properties of that function.

That'd work for me. Removes the temptation to make the resolver itself a promise, which Legendary does at the moment.

I guess the issue is whether you are dealing with maybe-promises, or whether you know at all times whether you have a promise or a non-promise value. My experience has always been the latter. And I think most people have the same experience.

Yup, it's only for specific APIs that won't necessarily return a promise I bring out when(). Default assumption is non-foreign promises.

briancavalier commented 11 years ago

This rambling may belong in another issue, but ...

@kriskowal a while back I came full circle and now tend to agree with Mark about fulfill(promise). I have a branch in when.js that has fulfill functionality in it that allows a promise to be used as a value (as opposed to the "resolve"/"become"-like functionality that exists now), but haven't merged it because it made me a bit uncomfortable.

I've had one use case that might be helped (but not solved entirely) by it. I ended up solving it in another way, though, by wrapping/unwrapping a promise that I want to flow through a promise chain.

One reason it makes me uncomfortable is that fulfill(promise) "breaks" the identity function:

function identity(x) { return x; }

// true for any x
x === identity(x);

// Create a fulfilled promise
function fulfilled(x) {
  return new Promise(function(resolver) {
    resolver.fulfill(x);
  });
}

// Weirdness
// Assume ~~ is some magical equivalence operator for promises
// that compares the promise's fulfillment value.
// If x is not a promise, this is true
// But if x IS a promise, this is false!
fulfilled(x) ~~ fulfilled(x).then(identity);

Another reason is that promises already exhibit some nice functional/monad-like characteristics (whether they are or aren't monads is another topic), and monads have the property that M(x) == M(M(x)) (functional idempotence), i.e. you can't nest a monad instance inside a monad instance of the same type ... or more correctly, you can't observe the difference between the two, e.g. Maybe(x) and Maybe(Maybe(x)) are equivalent. Whereas you can observe the difference between fulfilled(promise) and fulfilled(fulfilled(promise))—just add .then(console.log) to each.

Yeah, I know, that's all theoretical, but there are good reasons for the identity function and for idempotence to continue to work. Of course, there may be other valid use cases and practical reasons for fulfill(promise), though, that others have encountered. Passing around decorated promises might be one, but I don't tend to do that.

unscriptable commented 11 years ago

KISS. It's possible to tunnel a promise through a promise chain by wrapping it in another object. If that's all that fulfill() does (tunnel a promise), then kill it now.

Is #5 a dup of this? Here are my thoughts on the promise constructor: https://github.com/promises-aplus/resolvers-spec/issues/5#issuecomment-11616725

ForbesLindesay commented 11 years ago

Comments from @unscriptable

This seems like a great way to achieve all of the desired features:

new Promise(function (resolve, reject) {
    resolve(promise);
    setTimeout(function () {
        reject(new Error('Operation timed out.'));
        // reject function has its own "extension" properties
        // so you could also do this (feel free to bikeshed these names):
        // resolve.createRejectedPromise(new Error('Operation timed out.'));
        // or this:
        // resolve.createCancellablePromise(promise);
        // or this:
        // resolve.createSomeObservablePromise(promise);
    }, 100);
  });

Pros:

  1. fulfillment is obvious and straightforward via function signature. 99% of the time, this is what devs will need
  2. first param can be treated as a resolver with several functions, even proprietary extensions
  3. KISS

Cons:

  1. duplicating the reject function arg as a resolve arg property might feel silly to some

Coments from @briancavalier

I like this hybrid. It makes the most common operations, resolve & reject, totally obvious and easy to use, while still allowing the resolve function (or reject, I suppose) to be used as the extension point for other to-be-spec'd and proprietary features.

briancavalier commented 11 years ago

Re tunneling: I agree. I'm no longer a fan of fulfill() as it's been described up to this point. It can only "tunnel" a promise through one step of a promise chain, which seems like it'll be a point of confusion. We can tell people that they need to keep returning fulfilled(promise) from their handlers, but they'll forget. And as @unscriptable said, I bet there are better solutions for cases where you think you need to tunnel.

So, right now I'm of the opinion that it's better for us just to say that the observable value of a promise will never be a promise, which, afaict, doesn't conflict with Promises/A+.

kriskowal commented 11 years ago

@briancavalier fulfill(x) cannot replace become(coerce(x)) or resolve(x). Promises/A+ cannot be implemented in terms of fulfill(x) because the return value of a function can be either a promise or a value. Whether the user interface is more or less learnable with or without fulfill is another story and I do not think that we will ever reach an ideal. At this point, I agree and think it’s best to retain resolve in function, if not by name.

briancavalier commented 11 years ago

@kriskowal Agreed, we probably can't reach an ideal for learnability :) I'm just feeling like fulfill creates more confusion than any value it might provide. Afaict, the only problem it solves is allowing you to tunnel a promise through one link in the chain. In my experience, that's so rarely useful that I think we shouldn't include it at all, and only spec resolve()/become(coerce()) style.

kriskowal commented 11 years ago

@briancavalier Just in case it is not clear, I agree that we should not specify fulfill.

domenic commented 11 years ago

I still think the resolve name is bad, though. Unless we stop using "resolved" to refer to "either fulfilled or rejected," the fact that you can call resolve(pendingPromise) and not actually resolve the promise is way too confusing.

I haven't fully digested the anti-fulfill arguments but my preferred solution at the moment is resolver that has magical "become" behavior when called as a function, but has reject and fulfill properties that behave like you'd expect.

briancavalier commented 11 years ago

@domenic Agreed: the name is confusing. I think we're all just using it out of habit right now :) People will tend to call it whatever they see us calling it, or whatever their current habit is.

lsmith commented 11 years ago

+1 to @ForbesLindesay's suggestion in this comment. Having the resolve arg be a function obviates the need for the spec to name it --it can be called fred in the implementation so long as it behaves correctly-- and makes the common case more terse. I'm less concerned about his listed con of having the reject function be duplicated as an argument and method on the resolve function object.

I don't think we'll find a natural name for what is currently known as resolve, since it means "finish or wait", which I don't know of any obvious English verb for, let alone one that would be such an overwhelming win over what is de facto. It's a confusing concept that is unlikely to be resolved (hehe) with a name.

lsmith commented 11 years ago

Oops, credit where credit is due correction: +1 to @unscriptable's suggestion in that comment :)

briancavalier commented 11 years ago

Another interesting thing about resolve(valueOrPromise) is that, technically, it does not require a separate reject(value) verb in the resolver API, since reject(value) === resolve(createRejectedPromise(value)). Of course, the rub is that you need a way to create a rejected promise ... or perhaps reject(value) is simply a convenience shortcut.

So, I'll pose the question: Given that resolve() is sufficient to implement reject(), does it make sense to specify only resolve() in the resolver API? That is: new Promise(function(resolve) {}). Other Promises/A+ functionality, like progress could simply hang off of the resolve() function. Libs could provide a reject() convenience function if they want.

Too crazy? Will that simply confuse people? Maybe reject() is a common enough thing that we feel like it needs to be specified?

juandopazo commented 11 years ago

I think function (resolve, reject) is way too nice from a didactic perspective.

lsmith commented 11 years ago

@briancavalier From the perspective of a spec, that does kind of make sense. Define the minimum functional requirement. Note optional, especially common/conventional features or signatures. Or maybe even spec them as "extensions". That said, it does seem to amount to choosing between defining the reject() function behavior in the context of Promise creation vs defining non-pending promise creation methods.

If there are other reasons to specify non-pending promise creation, then I'd be in favor of "core resolver spec" defining new Promise(function (resolve)), using the resolve function as an object host for other resolver methods, then noting that implementation MAY include a second arg as a direct reference to resolve.reject, defined as a convenience wrapper over returning a rejecting promise with the input value set as the reason.

If the only reasons for spec'ing non-pending promise creation are nice-to-haves, then it's more of a toss up. Defining the reject behavior directly seems like it would result in fewer run time operations and fewer objects, but I do find the notion of reject deferring to previously specified behavior of promises appealing.

domenic commented 11 years ago

I'm not entirely clear, but is the suggestion not specifying any way of creating rejected promises? That seems unfortunate.

briancavalier commented 11 years ago

@domenic No, that's not the suggestion. My point was simply that a single function as the resolver API, call it resolve() (whose behavior is similar to when.js/Q's current deferred.resolve()), is sufficient as long as you also have a way of creating an already-rejected promise. That is, if Promise.rejected(value) exists (however it might be implemented), you can simply do: resolve(Promise.rejected(value)) instead of reject(value).

So, as @lsmith pointed out, that raises the question of whether we should specify creation of already-rejected promises. If we do that, then perhaps our Resolver API should be a single resolve() function, and reject() should be optional. If we don't, then we have to specify reject().

Either way, yes, we need to specify some way of getting a rejected promise.

lsmith commented 11 years ago

@briancavalier Furthermore, including a reject argument in the signature might be construed as a means to bypass the logic whereby a passed promise's state is assumed by the "parent" promise. So we end up with

new Promise(function (resolve, reject) {
    reject(new Promise(...)); // aka rejectionPromise
}).then(null, function (reason) {
    // Is reason === rejectionPromise?
    // If rejectionPromise must be resolved before this being called,
    // is reason rejectionPromise's fulfillment value? What if rejectionPromise is rejected?
});

Which seems avoidable (in the "core" resolvers spec if there were such a thing) by not including reject in the signature.

It's likely that the rejection questions will need to be answered anyway, since technically today

promise.then(function () { throw otherPromise });

is possible. This may be an issue for the promises spec, which states in 3.2.6.2

If either onFulfilled or onRejected throws an exception, promise2 must be rejected with the thrown exception as the reason.

"exception" is not defined in the spec.

lsmith commented 11 years ago

@briancavalier

Either way, yes, we need to specify some way of getting a rejected promise.

Specifying reject() in my mind translates as "some way to transition a pending promise to a rejected state", not "getting a rejected promise". With reject(), you can add a convenience to get a rejected promise, but that functionality wouldn't be core.

kriskowal commented 11 years ago

@lsmith, perhaps it could be more clear that "the thrown exception" implies "otherPromise" in the expression "throw otherPromise". It most certainly is mandatory.

lsmith commented 11 years ago

@kriskowal what exactly is mandatory? Should the promise's rejection reason be otherPromise? Ticket filed here

ForbesLindesay commented 11 years ago

When we add things like long stack traces, creating a promise can start to be costly in performance terms (if there's no need to do so).

As such I'm in favor of having a way to transition a pending promise into a rejected state without creating an extra promise just to pass to resolve.

In addition I find adding the convenience method to create an already rejected promise by creating a pending promise and then rejecting it much more natural than creating a method for rejecting a pending promise by creating a rejected promise and passing it to resolve.

terinjokes commented 11 years ago

In #19 and in IRC, @domenic mentioned a different possiblity that I like:

function resolverFunc(resolve, reject) {
// …
}

function onCancelled(cancellationToken) {
// …
}

new Promise(resolverFunc, onCancelled);

This keeps the resolver function pretty straight-forward.

ForbesLindesay commented 11 years ago

The problem with that is that a typical web request might look like:

function get(url) {
  var req;
  return new Promise(function (resolve, reject, progress) {
    req = http.get(url);
    req.on('response', resolve);
    req.on('error', reject);
    req.on('progress', progress);
  }, function onCancelled() {
    req.cancel();
  });
}

This scoping issue is kind of ugly. If you go for the other alternative you might get:

function get(url) {
  return new Promise(function (resolve, reject, progress, onCancelled) {
    var req = http.get(url);
    req.on('response', resolve);
    req.on('error', reject);
    req.on('progress', progress);
    onCancelled(function () {
      req.cancel();
    });
  });
}

This looks a lot cleaner to me, since you don't have to artificially hoist the req variable just to get cancellation.

terinjokes commented 11 years ago

I'm not personally bothered too much about raising the scope of req, but I understand that's very much a personal preference. I definitely don't like having an unknown amount of future extensions hanging off the end of resolverFunc.

Another idea above proposed having a third parameter (extensions or options) that held the possible extensions? This simplifies resolverFunc to always being 3 parameters and is similar to what Node and client-side developers already do for some library callbacks.