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

Both when() callbacks can get triggered. #75

Closed
danfuzz opened this issue May 22, 2012 · 5 comments
Closed

Both when() callbacks can get triggered. #75

danfuzz opened this issue May 22, 2012 · 5 comments

Comments

@danfuzz
Copy link

danfuzz commented May 22, 2012

I'm not entirely sure if what I've run into is a bug in Q per se, or just weird behavior because I'm mis-using it, but in any case, I've constructed an example where both the resolved and rejected callbacks of a when will get called. My understanding is that only one or the other should ever get called.

All you need to do to get the bad behavior is define a custom promise which always responds to when() by returning a rejection, as opposed to calling the passed-in rejected function. See below.

My best guess is that this isn't the recommended way to write a when() though I would have also guessed that it should still work. BTW, I noticed that in the inner implementation, it doesn't count on rejected being passed in, but at the level of my example here, is it actually safe to assume that it's a real function?

Here's the code:

var Q = require("q");

function makePromiseCore() {
    function post(name, args) {
        return "hello";
    }

    function when(rejected) {
        return Q.reject(new Error("permanently unresolved"));
    }

    return {
        post: post,
        when: when
    };
}

function onResolved(value) {
    console.log("Resolved to:", value);
}

function onRejected(value) {
    console.log("Rejected as:", value);
}

var promise = Q.makePromise(makePromiseCore());
promise.when(onResolved, onRejected);

And here's a transcript of running it:

$ node ./weirdwhen.js 
Rejected as: [Error: permanently unresolved]
Resolved to: undefined

Thanks for your consideration. I'm generally quite enthused with this module!

@danfuzz
Copy link
Author

danfuzz commented May 22, 2012

FWIW, I just looked at the Q implementation a bit closer and figured out that you get this same behavior without defining when() in the descriptor at all, since the default rejector is written in exactly the same way. That is, you can comment out the line when: when in my example and still see the bad behavior.

Also, to be clear, I did these tests with version 0.8.5 as fetched via npm.

@ForbesLindesay
Copy link
Collaborator

promise.when is not really the primary intended method for resolving a promise, instead you should really use either:

promise.then(onResolved,onRejected);
//or
Q.when(promise, onResolved, onRejected);

However both these exhibit the same behaviour, so that's not the problem. I've submitted a pull request which fixes this, but since makePromise is an advanced feature, I'll have to defer to the author for how it's meant to be used.

@danfuzz
Copy link
Author

danfuzz commented May 23, 2012

Total sidebar here:

I thought promise.when() and promise.then() were synonyms. I'm betraying my prior work experience (on E, quite a while ago) by having picked that over promise.then() in my example.

Thanks for the recommendation, and thanks for the fix.

@kriskowal
Copy link
Owner

@danfuzz, In Q, promise.when is an alias that forwards to promise.then. promise.then in turn is implemented with Q.when. Internally, this uses the "when" message. The words bear the same meaning, but the implementation is wishy-washy about which work should be used for static (Q.when) and dynamic (promise.then, (but also aliased as promise.when)). Q assimilates objects that implement "then" (thenables) into Q promises, but it does not do the same for "whenables" because such things do not exist in the wild.

It’s a bit of a mess, being stuck between the trends in JavaScript and trends in forebears like E.

My inclination is to keep Q.when and promise.then, and remove promise.when so at least you can go by the rule "what makes sense when spoken", but on the other hand, MarkM’s favoring Q(promise).when in the Concurrency Strawman, which I would like to support.

What I mean by the mnemonic, these are the natural choices of words, and the corresponding implied usage in Q.

"When A, B", Q.when(A, B)

"A then B", A.then(B)

@ForbesLindesay
Copy link
Collaborator

I do really like the idea of sticking to the natural choices of words, I think it might be a nice idea to go with something like Q(promise).then though as the static version, that way they'd both be then.

domenic added a commit that referenced this issue Jul 25, 2012
See #76 for some discussion. The fix credit goes to @ForbesLindesay.
@domenic domenic closed this as completed Jul 25, 2012
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

4 participants