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

Accessing resolve/reject and promise at the same time #23

Open
domenic opened this issue Mar 4, 2013 · 9 comments
Open

Accessing resolve/reject and promise at the same time #23

domenic opened this issue Mar 4, 2013 · 9 comments

Comments

@domenic
Copy link
Member

domenic commented Mar 4, 2013

While implementing #18 as it stands, @wycats has run into the issue @erights outlined, of wanting access to both resolve/reject and promise at the same time. This is pretty common, so let's dig into it a bit more:

Solution 1: Awkwardness

var resolve;
var promise = new Promise((resolve_) => { resolve = resolve_; });
// use both `resolve` and `promise`.

Note that this only works if the resolver function is called synchronously (see #20).

Solution 2: Asyncness

var promise = new Promise((resolve) => {
  // use both `resolve` and `promise`
});

If the resolver function is called asynchronously (again, see #20), you can use both at once.

Solution 3: this

var promise = new Promise(function (resolve) {
  // use both `resolve` and `this`,
  // where `this` becomes `promise` after the constructor returns
});

@wycats's solution was to bind the promise-to-be-returned as this, i.e. as an implicit zeroth argument inside the resolver function. I don't like this pattern in general, and think it's a bit unfortunate that such a pattern would require not using ES6 arrow functions. But some people like it, including one of our favorite implementers, so, meh?

Solution 4: extra param

var promise = new Promise(function (resolve, reject, promise) {
  // use both `resolve` and `promise`
});

A variation on solution 3, this makes the parameter explicit instead of implicit, as suggested by @briancavalier.

@ForbesLindesay
Copy link
Member

I'm generally anti this as an implicit zeroth argument, but as what is essentially part of the constructor, I'm broadly in favor of it. You could use arrow functions whenever you didn't need access to this still.

In addition, I think we should decide soon whether we are going to call that function synchronously or asynchronously so that if someone picks one of the earlier methods it either always works or always fails

@domenic
Copy link
Member Author

domenic commented Mar 4, 2013

I think people are generally against calling the resolver asynchronously, which is a bit unfortunate, but sensible.

@erights had a great comment outlining our options from there. I think we're probably going to end up with a synchronous promise constructor + asynchronous assimilation.

So that would leave us with solution 2 and 3. If other people like the promise-as-this idea, or at least don't hate it, I guess we'll fly with it.

@erights
Copy link

erights commented Mar 4, 2013

I don't hate the promise-as-this idea for the factory function. On odd numbered days I like it.

I like Q.promise being sync and Q(thenable) assimilation being async. The implementation of Q at https://code.google.com/p/es-lab/source/browse/trunk/src/ses/makeQ.js now does this. (It doesn't do the promise-as-this yet.)

@briancavalier
Copy link
Member

Since it feels like part of a constructor, as @ForbesLindesay said, I don't hate the promise-as-this idea, but I also don't really like it. If it gets the majority support here, I'll go along with it, but my preference would be to find another way.

Why not make it explicitly a parameter? I know it adds to the number of params, but if we're willing to make it an implicit this param, why not go the extra step and make it explicit? People can ignore it if they want.

@domenic
Copy link
Member Author

domenic commented Mar 5, 2013

@briancavalier I kind of like it, and added it to the original post. But, it's a bit weird, because it breaks symmetry with then. I.e. if we were to specify onProgress as a third parameter to then, that wouldn't match. And even if we don't, one of the stronger arguments for (resolve, reject) versus ({ resolve, reject }) is that (resolve, reject) matches how then works, whereas the argument seems less strong for (resolve, reject, promise).

@ForbesLindesay
Copy link
Member

I really think we'll end up with (resolve, reject, progress, handleCancel), as such I'm really not sure where promise fits, other than this. If we're guaranteeing synchronous behavior I don't really have anything against:

var resolve;
var promise = new Promise((resolve_) => { resolve = resolve_; });

We can effortlessly wrap it up in a convenience method:

function defer() {
  var resolve, reject, progress;
  var promise = new Promise((_resolve, _reject, _progress) => {
    resolve = _resolve;
    reject = _reject;
    progress = _progress;
  });
  return {promise: promise, resolve: resolve, progress: progress};
}

I think we should just encourage library authors to provide that as a convenience method.

@juandopazo
Copy link

I never intended the initialization function to be a parallel to then. I always understood it more like an event listener, something along the lines of http.createServer(fn) in Node. That's why I chose to make this point to the promise. Another reason is that you may want to call other methods during initialization if you're using an extended promise.

@briancavalier
Copy link
Member

@domenic Since we're mandating that this is not provided for then callbacks, by forcing this in the constructor callback, it seems like we'd also be breaking symmetry with then(), but imho, in a sneakier way. If we go with a sync callback, we're also breaking symmetry with then(), so maybe we should just abandon the idea that the callback and then should have any intentional notion of symmetry?

I'm not really sure what to do about progress and onCancelled at this point, though :/. I still feel like this a bit of a slippery slope. Even though we don't see a potential 5th parameter right now, it seems hard to say whether or not in 2 months (or 6 months, or 12 months) we'll come up with something--and we will have already set the precendent for 4 params, so why not then add a 5th, and 6th, etc. My preference would be to assume that we will come up with more, and design the API to account for that now.

fwiw, imho, I tend to agree with @erights that defer solves this problem better than the promise constructor, esp w/destructuring. If we specify that the promise constructor callback must be called sync, then that's a minimal spec that allows, as @ForbesLindesay pointed out, libraries to offer a defer-like solution for this (of course, then we have to tackle the question of whether we should spec it or not :) ).

@ForbesLindesay
Copy link
Member

My inclination is to say spec that it's sync, but leave the defer function in "Notes" as a recommendation. That way people who go "But how can I access the promise at the same time as the resolver" can be directed to the notes section, but not all implementers have to build it in the same way.

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

5 participants