Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ForbesLindesay opened this issue Dec 21, 2012 · 39 comments

Comments

@ForbesLindesay
Copy link
Member

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
Copy link
Member Author

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
Copy link

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
Copy link
Member Author

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
Copy link
Member

domenic commented Dec 21, 2012

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

@ForbesLindesay
Copy link
Member Author

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
Copy link
Member

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

@briancavalier
Copy link
Member

+1 for option 1

@briancavalier
Copy link
Member

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
Copy link

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

@kriskowal
Copy link
Member

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
Copy link
Member

domenic commented Dec 30, 2012

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

domenic commented Dec 30, 2012

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
Copy link

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
Copy link
Member

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
Copy link
Member

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: #5 (comment)

@ForbesLindesay
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

@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
Copy link
Member

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

@domenic
Copy link
Member

domenic commented Jan 3, 2013

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
Copy link
Member

@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
Copy link

lsmith commented Jan 14, 2013

+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
Copy link

lsmith commented Jan 14, 2013

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

@briancavalier
Copy link
Member

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
Copy link

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

@lsmith
Copy link

lsmith commented Jan 15, 2013

@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
Copy link
Member

domenic commented Jan 15, 2013

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

@briancavalier
Copy link
Member

@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
Copy link

lsmith commented Jan 15, 2013

@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
Copy link

lsmith commented Jan 15, 2013

@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
Copy link
Member

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

@lsmith
Copy link

lsmith commented Jan 15, 2013

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

@ForbesLindesay
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants