Propagate fulfilled value as it is #221

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
4 participants
Collaborator

rkatic commented Feb 25, 2013

Fulfilled value should be propagated as-it-is even if it is a thenable.

Collaborator

domenic commented Feb 25, 2013

This is in fact not correct; thenables should be recursively resolved. I'll let @kriskowal confirm, but generally the consensus has been that return thenableFor5 should act the same as return 5.

See also promises-aplus/promises-spec#76

Owner

kriskowal commented Feb 25, 2013

Yes, returning a thenable implicitly attempts to coerce that to a promise. Marking objects with "then" methods that are not indeed promises is probably the only legitimate use of Q.fulfill and one our forebears have probably not accounted for.

Collaborator

rkatic commented Feb 25, 2013

A resolved value is a resolved value, and it should be propagated as such. Returned values form callbacks, in other hand, should be recursively resolved. Otherwise Q is broken if resolved value is also a thenable.

Collaborator

domenic commented Feb 25, 2013

No, a fulfilled value is a fulfilled value; a resolved value is propagated.

Q is not "broken" if a resolved value is a thenable. Q is simply incompatible with thenables where then means something too different from what it does in promises. You should wrap them (e.g. deferred.resolve({ thenable: badThenable })) if you need to pass them through the promise chain.

Collaborator

rkatic commented Feb 25, 2013

Ok, I will use the fulfilled word then...
No, in this way Q does not support thenable fulfilled values regardless if its then works properly or not.

Collaborator

domenic commented Feb 25, 2013

Right, I believe that our view is that it's impossible for a thenable to have a legitimate fulfillment value that is a thenable; it is our responsibility to squash chains of "x represents something that represents y" into "x represents y".

Collaborator

rkatic commented Feb 25, 2013

it's impossible for a thenable to have a legitimate fulfillment value that is a thenable

Why not?! Why you are not willing to distinguish already fulfilled/rejected values from returned values/promises from callbacks?

Owner

kriskowal commented Feb 25, 2013

@rkatic It is not so much a question of willing as able. Detecting whether an object is a thenable or just happens to have a "then" method is very messy. If it is not a thenable promise, we should not be calling its then method, and even calling the then method is not sufficient in every case to distinguish it from a promise, a badly behaving promise, a poorly implemented promise, or not a promise at all.

Collaborator

rkatic commented Feb 25, 2013

The distinction that I am talking about doesn't go against Promise/A+.
Check my "experiment" lib that makes such distinction: https://github.com/rkatic/p

Collaborator

domenic commented Feb 25, 2013

@rkatic I believe it does go against Promises/A+ 1.1, though, as per the earlier-linked issue.

Collaborator

rkatic commented Feb 25, 2013

I am not buying this. Also I have not time to read all your discussions about. If you can point me out the reason of such drastic decision, I will be grateful.

Collaborator

domenic commented Feb 25, 2013

"Drastic," lol. This is an edge case by any definition.

I think I explained it rather well above. Promises represent values. Conceptually, promises for promises are nonsensical: they are chains of "x represents something that represents y," which really just should be collapsed to "x represents y." Furthermore, promises for promises are hazardous, but I won't bother trying to link you somewhere demonstrating this, since you are too busy to read any reasoning longer than a few sentences it seems.

As for the difference between "promise for promise for x" and "promise for thenable for x," @kriskowal explained this well above: we simply don't have the ability to tell the difference, so we assume in good faith that thenables are promises. Anyone who really needs to use non-promise thenables while also wanting to use a promise system will take the effort to wrap it in e.g. [nonPromiseThenable] or { thenable: nonPromiseThenable }.

Collaborator

rkatic commented Feb 25, 2013

Thanks for your effort @domenic. I am a lazy bastard. However, I still don't see why your arguments should be sound. When the onFulfilled(value) is called, value should be considered fulfilled, even if thenable. The intention is clear here to me.

Collaborator

domenic commented Feb 25, 2013

When the onFulfilled(value) is called, value should be considered fulfilled, even if thenable.

I agree. But only promise implementations call onFulfilled, and what we are arguing over in this thread---I thought---was what value should be. And my argument is that value should never be a thenable.

Collaborator

rkatic commented Feb 25, 2013

And my argument is that value should never be a thenable.

Well, then my question is what is your argument for your argument :)

Collaborator

domenic commented Feb 25, 2013

Sadly I don't think I can repeat myself any longer. You should never have a promise for a promise, as I tried to explain above, and we can't distinguish between promises and thenables, thus value should never be a thenable.

Collaborator

rkatic commented Feb 25, 2013

I guess I am really missing something, because for now, I only see circular reasoning here. I hope a sleep will help.

Collaborator

domenic commented Feb 25, 2013

Just in case, let me try to rephrase my most recent comment in a way that seems a bit less circular:

  1. Promises for promises are nonsensical.
  2. Therefore, promises should never have as their fulfillment value another promise. (We guarantee this by collapsing chains when assimilating, when dealing with return values from onFulfilled and onRejected, and by eliminating Q.fulfill as per #205.)
  3. We cannot distinguish between promises and thenables, therefore, promises should never have as their fulfillment value another thenable.
  4. Promise implementations call onFulfilled with the fulfillment value of a promise.
  5. If the fulfillment value can never be a thenable, onFulfilled will never be called with a thenable.

Ta-da! :)

Collaborator

rkatic commented Feb 26, 2013

You lost me on [1] :) Too meta for you? Not to me, for now at least.
The #205 I hoped was for Q.fulfill, but not for deferred.fulfill too.
[3] is correct iff you always have to distinguish between promises and fufilled values.
But again, I guess I am the one missing something, so will not expect other explanations for now.

Owner

kriskowal commented Feb 26, 2013

Let’s take a step back and sleep on this. @rkatic is right that no valid promise implementation would call onFulfilled with a promise, unless that promise is for an object that was fulfilled remotely and cannot become a value locally.

Collaborator

rkatic commented Feb 26, 2013

Now that I had my needed dose of rest, I have mixed feelings about this.

I can agree that not allowing thenable fufilled values makes the usage more "natural" in some cases, eliminating a subtle but sometimes important distinction to the user - user don't have to bother to return Q.fulfill(value) instead of simply returning the value, even when the nature of the value is unclear.

However, as mentioned by @kriskowal, it can be an too strong assumption that other promise implementations will never have thenable fulfilled values. Also, I am not sure that we can even consider this an edge case.
Imagine an ajax function that returns a thenable xhr. Since the xhr object stores various results on itself, it is also a logical candidate for the fulfilled value too. In fact, having promises that fulfills with itself could be considered a handy (anti)pattern (note that in such cases it is not that important to have a Q.fulfill).
However, I could be wrong here too, since I am relatively new on using promises in general.

Collaborator

rkatic commented Feb 28, 2013

Per discussion at #205, I suppose this one can be closed.

rkatic closed this Feb 28, 2013

Collaborator

rkatic commented Mar 1, 2013

Actually, since #205 is rejected, and acknowledging that fulfilled values can be thenables, this issue seams more relevant then before.
I also added a test for timeout that shows how such values are currently not propagated, but resolved, removing the effect of eventual return Q.fulfill(fakeThenable).
I renamed this issue to better describe both problems.

rkatic reopened this Mar 1, 2013

Collaborator

domenic commented Mar 1, 2013

Let's take a different approach.

Can you show us why this would be beneficial by including a test that (a) does not currently pass; and (b) is not about a cyclic promise chain (i.e. a promise resolving to itself)?

"should propagate fulfilled value as it is" currently passes, without your code modifications.

"should coerce thenables which resoved value is also thenable" is actually about promise cycles, which is covered by #223.

Collaborator

rkatic commented Mar 1, 2013

@domenic Both tests that I added do not currently pass. Not completely sure what you mean with "not a cyclic promise chain", but the second added test should not be one of those.

Collaborator

domenic commented Mar 1, 2013

Oh, sorry for the wrong assumption then. The fact that "should propagate fulfilled value as it is" doesn't pass seems worrying and is a convincing argument. I'll try to look into it.

Thanks for your persistence.

Collaborator

rkatic commented Mar 13, 2013

Are you ok with removing changes regarding coercing? Would it make this pull-request more acceptable?

Collaborator

domenic commented Mar 13, 2013

I think we're going to wait until the Promises/A+ test suite 1.1 is done, which contains tests to ensure that returned thenable-for-thenable chains are flattened. Q already passes those, but having them in place will prevent regressions from pull requests like this. Then we can investigate how to make the tests you included pass.

I should be done with the test suite by this weekend, most likely.

Collaborator

rkatic commented Mar 13, 2013

Because of the recursive assimilation algorithm in Promises/A+ 1.1, preventing resolution to stuck in every situations is not simple, if not impossible (depends on how badly the thenable then behave). I think that only thenables fulfilled with itself should be eventually supported. Anything more would require an array to detect cycles (ignoring cases of very bad thens), and/or stopping assimilation on a fixed "max-assimilation-recursion".

Owner

kriskowal commented Jun 30, 2013

In the time since this issue opened there was extensive deliberation on recursive flattening and monads on ES-Discuss. @domenic, @erights, what was the resolution of that discussion and how does it apply to Q?

Collaborator

erights commented Jun 30, 2013

It looks like we'll be settling on a variant of AP2 (see http://wiki.ecmascript.org/lib/exe/fetch.php?id=strawman%3Apromises&media=strawman:promisesvsmonads2.pdf ). But don't change Q or Promises/A+ yet. This should all be decided at the end-of-July TC39 meeting.

When we have a decent draft of this AP2 variant, I'll post a pointer here and on Promises/A+ and invite comments.

Collaborator

domenic commented Jul 2, 2013

FWIW Promises/A+ 1.1 is wrapping up soon, so my strategy of waiting until we have those tests in place, then trying to merge this because it contains some tests-that-fail-but-probably-shouldn't, still seems like the way to go.

Owner

kriskowal commented Oct 20, 2013

Reminder. The ink is drying on the ES6 promise specification. Let’s note the decisions here and wrap it up.

kriskowal closed this Feb 10, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment