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

Add normative text and supporting note for handler-returned promises #52

Merged
merged 2 commits into from
Dec 5, 2012

Conversation

briancavalier
Copy link
Member

This will probably conflict with #49, so either @lsmith or I will need to rebase. Wanted to get feedback, though.

@domenic
Copy link
Member

domenic commented Nov 27, 2012

I like it, but I think implementer feedback and/or a survey of existing libraries is needed to see if this actually matches how things are done, in practice. If there's another way to make this work that people are doing, we don't want to prohibit it.

Thus, I summon @briancavalier @kriskowal @wycats.


As an aside, underscore.deferred basically doesn't chain at all with non-underscore.deferred promises. So it's actually really good that we're specifying this.

@kriskowal
Copy link
Member

@domenic, let’s make a test for assimilation between libraries A and B, then run it on the clique of adapters and see what happens. The language is stronger than necessary for cannibalism and suboptimal for Q since it has other messages than just "then", but the language is quite sufficient for assimilation.

@briancavalier
Copy link
Member Author

I think the language here is compatible with when.js's implementation. In all cases, when.js does call returnedPromise.then(f, r), where f and r may eventually lead to the fulfillment/rejection of promise2. Specifically in cases where returnedPromise is "untrusted", there is another level of indirection (as there probably would be with any implementation that assimilates), but I think still compatible.

In addition to @kriskowal's library-pair-permutation idea, you could also create a small set of "fake" promises for libs to attempt to assimilate.

@briancavalier
Copy link
Member Author

I'll rebase this and we can continue to discuss.

@domenic
Copy link
Member

domenic commented Dec 3, 2012

OK, actually this language is a bit too restrictive for Q, as @kriskowal points out. In particular, for Q promises, we bypass then directly and use the internal promiseSend protocol.

In practice, this means that if I wrote a test that used a Q promise as returnedPromise, but spied on its then method to ensure it was called with something, it would fail. Hmm.

Having a hard time thinking of what to do here :-/. We could move the attempt-procedure entirely into the non-normative section, I guess.

@briancavalier
Copy link
Member Author

Hmmm, yes, I see the problem. If a particular implementation recognizes other types of promises and prefers their API over Promises/A+, it makes the implementation non-compliant, even if it does call then on other, "pure" Promises/A+ promises. In fact, this prevents any implementation from using an (possibly more efficient) implementation-specific mechanism for achieving the correct outcome.

The text of these particular points is very prescriptive in terms of implementation--more so than the rest of the spec. It seems like sticking with the original language, which dictates only the correct outcome, and adding a note would be ok.

Another option might be to reword the text to keep "must" for "into the same state", but use "should" for the portion that deals with calling returnedPromise.then.

Other ideas?

@lsmith
Copy link
Contributor

lsmith commented Dec 3, 2012

@briancavalier +1 to keeping the existing language and adding a note, though I admit the note doesn't feel necessary to me. What's important here is the outcome.

The obvious choice for implementers, given the criteria on lines 67+, is to call the promise's then method, IMO. If implementers want to achieve the ends by different means, e.g. Q, that's fine, but to be resilient of outside A+-compliant promises, they must fall back to then if things like promiseSend aren't supported by the instance because the only thing definitive about A+ promise composition is that they are objects/functions with a then method.

@domenic Tough to test, I agree.

@briancavalier
Copy link
Member Author

@domenic If you're also ok with the original language + note, I'll update the pull request.

@domenic
Copy link
Member

domenic commented Dec 3, 2012

@briancavalier sure, give it a shot.

@briancavalier
Copy link
Member Author

Ok guys, let me know what you think. The note is a bit long, but seems necessary to include everything that's in there.

@lsmith
Copy link
Contributor

lsmith commented Dec 4, 2012

LGTM

@kriskowal
Copy link
Member

Yeah, looks fine from here.

@domenic domenic merged commit 2486935 into master Dec 5, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants