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

Small clarification on what is a legal value #44

Closed
awwx opened this issue Nov 17, 2012 · 24 comments
Closed

Small clarification on what is a legal value #44

awwx opened this issue Nov 17, 2012 · 24 comments

Comments

@awwx
Copy link

awwx commented Nov 17, 2012

The spec says

  • a “value” is any legal language value, including undefined, that is not a promise
  • A promise is an object or function that defines a then method that accepts [...] two arguments [which must follow this specification]...

and specifies that a callback has different behavior depending on whether a value or a promise is returned.

I might attempt to construct what I think would be a legal value:

{
  "at": function () {},
  "from": function () {},
  "then": function() {},
  "was": function() {}
}

which I think is not a promise because it doesn't meet the requirements to make it a conforming promise; and since it's not a promise then it should be a legal value.

Of course one couldn't expect an implementation to inspect my object to see that it wasn't a conforming promise... so for clarity it would be useful to say something like a value is "any legal language value... that is not an object with a then property which is callable."

@awwx
Copy link
Author

awwx commented Nov 17, 2012

Actually, since conforming implementations must be able to detect whether an object is or is not a promise created by another implementation (correct me if I'm wrong), the test for "a value" would need to be implementable, and thus should probably be something like "not an object with a then property which has a typeof 'function'".

@briancavalier
Copy link
Member

Hmm, it seems like the spec says exactly that:

A promise is an object or function that defines a then method

and

"value" is any legal language value, including undefined, that is not a promise

So, by definition a value is anything that is not an object or function, or is an object or function, but does not define a method (i.e. a callable property) named then. IMHO, the current definition of value is sufficient.

Although, one nit may be that the phrase "which accepts the following two arguments" is not something we intend to enforce and is not something that is checked in practice. It's possible some may take it to mean, as I think @awwx is hinting, that an object or function with a callable then which doesn't accept at least two arguments is a value (and not a promise).

In practice, the check for a promise used by most Promises/A compliant libs is:

x && typeof x.then === 'function'

I'm not aware of any libs that check x.then.length. I don't feel a strong need to clarify it any further, but let's see what others think. @domenic?

@ForbesLindesay
Copy link
Member

The problem is that { then: function (cb, eb, progback) {...}} should be a promise so if someone impliments the spec and interprets that they need to chcek the length of the fb, it won't work

@awwx
Copy link
Author

awwx commented Nov 17, 2012

hinting, that an object or function with a callable then which doesn't accept at least two arguments is a value (and not a promise

No, actually, the question is more what happens to {"then": function () {}} (regardless of how many arguments it accepts).

Suppose "a promise" is defined as "an object with a then method". By that definition {"then": function () {}} is a promise.

Ok, but

2.2.6.5 If either onFulfilled or onRejected returns a promise (call it returnedPromise), promise2 must be placed into the same state as returnedPromise

so now if I return {"then": function () {}} from a callback then you are required to place "promise2 into the same state" as the object I returned, which you can't do.

Conversely, the spec as it is currently written says "A promise is an object or function that defines a then method that accepts [onFulfilled and onRejected]" and further specifies how onFulfilled and onRejected must work... which could be interpreted to say that if the then method doesn't work that way, the object is not a promise. But... if {"then": function () {}} is not a promise, then it is a value, you're supposed to fulfill promise2 with that value, and you aren't going to be able to do that either.

How to say that {"then": function () {}} is not a valid value and it is not a valid promise?

One suggestion is that "a promise" could mean "an object fully compliant with the Promises/A+" spec and that "a value" is "a legal language value for which x && typeof x.then === 'function' is false".

Or, you might prefer to say that "a promise" is a value for which x && typeof x.then === 'function' is true, a value is a legal language value that is not a promise... and you introduce another concept such as a "valid promise" which is an object which conforms to Promises/A+, and then for example in 2.2.6.5 you could say that you only have to put "promise2 into the same state" for a valid promise.

Either way you avoid the ambiguity around {"then": function () {}}.

@domenic
Copy link
Member

domenic commented Nov 18, 2012

Hmm. I think this is a good point to bring up, and some interesting clarifications have been made. Notably, when I talked with @lukehoban (T39 member) about standardizing promises, he brought up that one of the things he thought was missing from Promises/A+ was a more precise definition of "promise." Partially this was regarding the duck-typing versus canonical-constructor question that standardizing promises at a language level would bring up, but it also is telling about a potential hole in the spec. Perhaps clarifying with a line item specifically saying that promises are objects or functions with a then method would be good, to avoid the argument-arity questions.

Let's recall the overall goal of such exercises, though: we need to find the edge cases and specify them so users know how promise libraries will behave. In particular, solutions that make the spec more airtight by walling of large chunks of reality into "noncomformant land" are not great, so I dislike introducing concepts like "valid promise."

My first reaction was that this was the situation compliance branding (see #2) was meant to solve: we could specify precisely what an A+ promise is in that way. But that doesn't really help. We want to have users be able to return crap-promises and still a Promises/A+ library does its best to do the right thing. For example,

var p = aplusPromise.then(function () {
    return $.when(5);
});

should result in p being fulfilled with the value 5.


Libraries essentially accomplish this by saying if the return value has a then method, it is a promise (call it returnedPromise) and then they place promise2 in the same state as returnedPromise by doing returnedPromise.then(fulfillPromise2, rejectedPromise2).

I suppose we could spec this. We could modify the main text to more precisely state what "place in the same state" means, along the lines of:

If either onFulfilled or onRejected returns a promise (call it returnedPromise), returnedPromise's then method must be called with a first parameter that fulfills promise2 with the value of its first argument, and with a second parameter that rejects promise2 with the value of its second argument.

This is pretty icky-sounding and I hesitate to type it because now nobody will try and think of a better phrasing, but there it is.

Alternately we could expand upon this in a footnote, saying something like

Since promises are defined simply by the presence of their then method, in practice aligning the state of promise2 with returnedPromise means calling returnedPromise.then(fulfillPromise2, rejectPromise2), where the arguments are functions that do what their names imply.

@awwx
Copy link
Author

awwx commented Nov 18, 2012

Yes, if it works to have 2.2.6.5 specify what an implementation should do instead of what the result should be, that also fixes the issue for an implementation that wants to be compliant.

@briancavalier
Copy link
Member

o i c what you are asking, I misunderstood your original question, @awwx. So yeah, I think we could use a note along the lines of what @domenic suggested, to provide guidance for implementors there.

It's worth noting though, that { then: function() {} } is a promise, but one whose implementation is completely busted :) We can't really account for that. One thought is that we could add some language to the spec, possibly also in a note that says something like:

In any program using Promises/A+ promises, the following test must be sufficient to distinguish a promise from a value: x && typeof x.then === 'function'

Or something along those lines. That sort of implies that to use promises, you need to make sure your code isn't returning something like { then: function() }

@awwx
Copy link
Author

awwx commented Nov 18, 2012

Yeah, issue one is that for the end-user, they should know that returning {then: function () {}} from a callback is a bad idea... but that wasn't entirely clear (to me at least when I first read the spec). That is, I actually did know it was a bad idea :), but from the spec it sounded like maybe I was supposed to be able to do it.

Issue two is the wording of 2.2.6.5 could be interpreted to mean the promises implementer was supposed to get it to work, when of course they can't :)

So I think if you say that a promise is a value that satisfies x && typeof x.then === 'function', and you change 2.2.6.5 to say that the implementation calls the "promises"'s then method, then you'll be all set: the user knows that a) { then: function() {} } is (technically) a promise, and b) it will break if they use it. And the implementer isn't (theoretically) supposed to achieve the impossible :)

@domenic
Copy link
Member

domenic commented Nov 18, 2012

More comments later, but just wanted to point out one interesting thing:

So I think if you say that a promise is a value that satisfies x && typeof x.then === 'function'

I don't think this is what you want, given that such a test will fail the (admittedly silly)

var x = { valueOf: function () { return false; }, then: function () { } };

Instead it should probably be

typeof x === "object" && x !== null && typeof x.then === "function"

or in ES-spec language (for fun)

  1. If Type(x) is not Object, return false.
  2. Let then be the result of calling the [[Get]] internal method of object x with argument "then".
  3. Return IsCallable(then).

@medikoo
Copy link

medikoo commented Nov 19, 2012

@domenic mind that in some of the already existing implementations promises are functions, so typeof x === 'object' will fail.

I was thinking about it recently, and I think that aside of then existence check (just that is error-prone), we should have something more that identifies the promise. Maybe we could agree on some static property that should be provided on each promise. e.g. promise.isPromise === true?
Then check would be as simple as that:

x && (typeof x.then === 'function') && (x.isPromise === true)

Looking at ways of how we identify native EcmaScript objects, the best solution would be following:

Object.prototype.toString.call(promise) === '[object Promise]'

but we're unable to implement such behavior, so it's good to come up with some workaround.

@domenic
Copy link
Member

domenic commented Nov 19, 2012

@medikoo Good point! My ES-spec language version is correct but not my JS version. Not sure what that says about me.

New version:

(typeof x === "object" || typeof x === "function") && x !== null && typeof x.then === "function"

I was thinking about it recently, and I think that aside of then existence check (just that is error-prone), we should have something more that identifies the promise.

See #2!! But then, see my above argument: we still want to be able to support returning crappy not-even-Promises/A promises.


Everyone: another possible problem to specify. What do we do in this new spec-addition for the case of

{ then: function () { throw new Error("boo!"); } }

? We should probably cover this case.

Unfortunately it seems like we're having to spec an entire assimilation protocol (i.e. a when function) just to handle chaining correctly. Annoying! But probably necessary??

@medikoo
Copy link

medikoo commented Nov 19, 2012

@domenic nice, then.aplus indeed looks like a possible solution

@ForbesLindesay
Copy link
Member

Perhaps we could say that an implementation must do the magic with promises/A+ promises and should make a best effort attempt with other promises.

Perhaps we could agree to have a canonical assimilate-promises implementation that would turn non-compliant promises into a bare-bones compliant-promise, that libraries such as Q could then extend.

@briancavalier
Copy link
Member

I dunno guys. Any duck-type test can be thwarted. Simply adding more checks makes it less likely someone will accidentally do it, but until we have a language-level check, it can't be foolproof.

{ then: function () { throw new Error("boo!"); } } is a busted promise implementation. I don't think we should account for stuff like this. If it duck-types as a promise, the we should treat it as such, and if, as a result, it fails in unpredictable ways, so be it. Similarly for implementing a nonsensical valueOf. These haven't been problems in practice (depending on how you view jQuery.Deferred, I suppose)

I would rather provide guidance, or even specify (see upthread) what .then() should do when a handler returns a promise. I don't see why we need to go any further than that.

Codifying the test that should separate promises from values does seem like a good addition. We could tighten this test slightly to something like

x != null && typeof x.then === 'function'

Or something like the very narrow version that @domenic suggested, which exactly matches the current spec language "A promise is an object or function that defines a then method":

(typeof x === "object" || typeof x === "function") && x !== null && typeof x.then === "function")

@domenic
Copy link
Member

domenic commented Nov 19, 2012

@briancavalier

{ then: function () { throw new Error("boo!"); } } is a busted promise implementation. I don't think we should account for stuff like this. If it duck-types as a promise, the we should treat it as such, and if, as a result, it fails in unpredictable ways, so be it.

Just to be clear, you're saying that the behavior for a handler returning this should be implementation-defined? :-/

@briancavalier
Copy link
Member

@domenic It's not clear to me how Promises/A+ could account for all possible busted promise doppelgangers. We could suggest/specify what a Promises/A+ .then should do in order to attempt to place the promise returned by .then "into the same state" as the handler-returned promise, but it seems impossible to account for all possible wrong implementations in handler-returned promises.

It seems like the simplest thing is for the spec to assume that handler-returned promises are well-behaved enough, and we can use their .then. It will always be possible for someone to return something that passes any duck type test, but whose implementation will wreak havoc, even if we assimilate it.

@briancavalier
Copy link
Member

Where do we stand on this one? Is @domenic's latest addition of defining the term "promise" enough, or do we still need to go further? Specifically:

  1. Do we need/want to explicitly provide an "official" duck-type test code snippet?
  2. Do we need/want to specify/suggest the way in which implementations should make a best attempt at placing a promise "into the same state" as a handler-returned promise?

I think 1 could be helpful, but I'm not sure about 2. Thoughts?

@domenic
Copy link
Member

domenic commented Nov 26, 2012

I think 1 is not necessary given that it would just be restating the definition in code. We could add it as a footnote if we wanted, I guess.

I think 2 is very necessary.

@briancavalier
Copy link
Member

Wording 2 might be a bit tricky since we're staying away from fulfill/reject APIs. We may be able to say something generic like calling the handler-returned promise's then method like so: returnedPromise.then(<function to fulfill promise2>, <function to reject promise2>) or something along those lines.

@domenic
Copy link
Member

domenic commented Nov 26, 2012

Yeah. It might be the place for a non-normative note explaining all the subtleties. Maybe

Given that returnedPromise may not be Promises/A+-compliant, but could instead be any object with a then method, it isn't always possible to fulfill the requirement of placing promise2 in the same state as returnedPromise. In this case, a best-faith effort should be attempted, along the lines of returnedPromise.then(<function to fulfill promise2>, <function to reject promise2>).

Alternately, we could be more normative about it:

If either onFulfilled or onRejected returns a promise (call it returnedPromise), the implementation must attempt to place promise2 into the same state as returnedPromise by calling returnedPromise.then with the following parameters:

  1. A function who fulfills promise2 with the value of its first parameter.
  2. A function who rejects promise2 with the value of its second parameter.

Maybe there "attempt" could have a footnote explaining why it's an attempt.

@briancavalier
Copy link
Member

I like those. Perhaps we combine them, as you suggested.

Normative:

If either onFulfilled or onRejected returns a promise (call it returnedPromise), the implementation must attempt [3] to place promise2 into the same state as returnedPromise by calling returnedPromise.then(fulfillPromise2, rejectPromise2), where:

  1. fulfillPromise2 is a function which fulfills promise2 with the value of its first parameter.
  2. rejectPromise2 is a function which rejects promise2 with the value of its first parameter.

Footnote for "attempt [3]"

Given that returnedPromise may not be Promises/A+-compliant, but could instead be any object with a then method, it isn't always possible to fulfill the requirement of placing promise2 in the same state as returnedPromise. Thus, the procedure here represents a best-faith effort.

@ForbesLindesay
Copy link
Member

What about things that are even less like promises, e.g. an event emitter which might have two events error and done?

@domenic
Copy link
Member

domenic commented Nov 26, 2012

@briancavalier Looks pretty good. Only remaining questions:

  • Can we somehow get rid of the word "implementation"? We don't use it anywhere else, so it'd be a shame to introduce that "new concept" here.
  • I assume this completely subsumes all existing bullet points under 2.2.6.5?

@ForbesLindesay Those fall under 2.2.6.3: "If either onFulfilled or onRejected returns a value, promise2 must be fulfilled with that value." The only issue here is promises (1.1: ""promise" is an object or function that defines a then method.") that aren't A+-compliant.

@domenic
Copy link
Member

domenic commented Dec 3, 2012

I think this can be closed in favor of #52.

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