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

Support promise arguments to proxy methods #152

Closed
SLaks opened this issue Dec 18, 2012 · 14 comments
Closed

Support promise arguments to proxy methods #152

SLaks opened this issue Dec 18, 2012 · 14 comments

Comments

@SLaks
Copy link
Collaborator

SLaks commented Dec 18, 2012

Background

I frequently need to write code that takes a value from one promise, and puts it into an object alongside a value from another promise.

For example:

getEmail(...)
    .then(function(email) {
        return {
            email: email,
            user: getUserInfo(email)  //promise
        };
    });

This obviously won't work, because it won't wait for the inner promise to finish.

I'd like to be able to do this without nesting callbacks.

Feature request

The closest option I can find within Q is the .put() method:

return Q.resolve({ email: email })
        .put('user', getUserInfo(email));

However, this doesn't work, because .put() doesn't resolve promises in its arguments.

The closest working code I can come up with is

return Q.spread(    //Horrible
    [ { email: email }, 'user', getUserInfo(email) ],
    Q.put
);

Can you change all of the proxy methods to resolve any promises in their argument lists?

@kriskowal
Copy link
Owner

I have been thinking about it for other reasons. We'll track the issue
here.

@SLaks
Copy link
Collaborator Author

SLaks commented Dec 18, 2012

Alternatively, can you think of any better solutions for the underlying problem?

For example,

getEmail(...)
    .then(function(email) {
        return Q.deepResolve({
            email: email,
            user: getUserInfo(email)  //promise
        });
    });

Maybe I'll add a deepResolve() method to my Qx library.

@domenic
Copy link
Collaborator

domenic commented Dec 20, 2012

I think deepResolve is pretty valuable and is a good solution to the original problem.

@SLaks
Copy link
Collaborator Author

SLaks commented Dec 20, 2012

Should it go in Q itself or in Qx?

@domenic
Copy link
Collaborator

domenic commented Dec 20, 2012

Or in QQ? :P

Up to @kriskowal though.

@SLaks
Copy link
Collaborator Author

SLaks commented Dec 20, 2012

It looks like QQ already has it.

https://github.com/kriskowal/qq/blob/master/qq.js#L215

@domenic
Copy link
Collaborator

domenic commented Dec 20, 2012

Yeah, but QQ is "out of service." Qx may be a good successor/replacement...

@SLaks
Copy link
Collaborator Author

SLaks commented Dec 27, 2012

I just realized that there is a much simpler solution to the original problem:

getEmail(...)
    .then(function(email) {
        return Q.all([ email, getUserInfo(email) ]);
    }).spread(function(email, user) {
        ...
    });

@ForbesLindesay
Copy link
Collaborator

I use that pattern all the time, and it serves me well. To make it equivallent to the original example you need to do:

getEmail(...)
    .then(function(email) {
        return Q.all([ email, getUserInfo(email) ]);
    }).spread(function(email, user) {
        return {email: email, user: user};
    });

which I assumed you felt was too clunky. It can be a little clunky and a deep-resolve would probably be useful in more complex scenarios.

@domenic
Copy link
Collaborator

domenic commented Dec 29, 2012

@kriskowal, @SLaks Are we happy with the solutions proposed here? Namely, slight-hackery with spread, or a utility library implementing deepResolve? If so we should close this up.

@kriskowal
Copy link
Owner

I would close this issue and open some new ones:

  • create a method akin to Q.all that replaces the properties of an object that are promises with their fulfillment values and fulfills to that selfsame object, or rejects on the first fail.
  • for further consideration: use Q.spread implicitly to resolve the arguments of all message dispatchers, thus promise.put(key, promiseForValue) would work, which might be useful for Q connection and has at least once been recommended by @Gozala. This would obviate the need for the Q.promised wrapper. I am rounding back to this idea. The reason for not doing this is that it becomes impossible (presently) for a promise to be passed as an argument without thening on it, which would be a problem for remote promises. However, I am thinking of adding a remote promise wrapper that would resolve to itself instantly, which would solve that problem. I’m also thinking of push and pull methods on promises.

@kriskowal
Copy link
Owner

Thanks @SLaks!

@Gozala
Copy link
Collaborator

Gozala commented Dec 29, 2012

I personally like promised decorator as it reduces API surface one need to learn for working with promises
to an absolute minimum which is promised itself for all transformations and then for final consumption.
For example problem described above could have being solved as follows:

var getUserInfo = promised(rawGetUserInfo)
var user = promised(function(email, user) {
  return { email: email, user: user }
})

var email = getEmail(...)
var myUser = user(email, getUserInfo(email))

Also it's very likely to have some utility functions in toolbox already that can
compose without any extra code:

var getUserInfo = promised(rawGetUserInfo)
var user = promised(record("email", "user"))
var email = getEmail()
var myUser = user(email, getUserInfo(email)

where record is:

function record() {
  var names = Array.prototype.slice.call(arguments)
  return function() {
    var values = arguments
    return reduce.names(function(result, name, index) {
      result[name] = values[index]
      return result
    }, {})
  }
}

@Gozala
Copy link
Collaborator

Gozala commented Dec 29, 2012

Another interesting consequence of describing computations via promised functions is that
any synchronous computation can be easily turned into async by just decorating functions involved
in that computation.

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