promises-aplus / promises-spec

An open standard for sound, interoperable JavaScript promises—by implementers, for implementers.
https://promisesaplus.com/
Creative Commons Zero v1.0 Universal
1.84k stars 163 forks source link

Ratify 1.1 #84

Closed domenic closed 11 years ago

domenic commented 11 years ago

I'd like to get signoff at least from the following people before publishing 1.1 to gh-pages. If I forgot your name, please don't feel offended!!


You can check out the full diff at this GitHub compare link, and you can see the current-and-hopefully-soon-to-be-1.1 text in master, but the major changes you are ratifying are:

This last point is probably a breaking change for existing libraries. It amounts to using thenable.then(resolve, reject) instead of thenable.then(fulfill, reject) in your assimilation code, and extra edge-case handling. The test suite currently does not cover this point, but of course will if we move forward.


This is also a great time to do some word-smithing. I am especially curious if anyone can do a better job with the Promise Resolution Algorithm. We also have #80 outstanding regarding possibly replacing "fulfillment value" and "rejection reason" with simply "value" and "exception."

Excited!! Let's go!!

ForbesLindesay commented 11 years ago

Once #83 is resolved, it'll get my signoff.

ForbesLindesay commented 11 years ago

Right, as it currently stands, it has my signoff (I'm not a collaborator, so I can't tick my own box)

domenic commented 11 years ago

Oh, if people could weigh in on #80, that'd be swell.

briancavalier commented 11 years ago

Releasing 1.1 will definitely "revoke" many libraries' current compliant status (avow.js included). I'm actually ok with that, because I think it's a good forcing function for people to improve their implementations. However, it does raise the question of logistics: we may need to drop some suggestions to people (after a grace period) that they need to update their lib.

kriskowal commented 11 years ago

I realize that this work is all on master. Can we contrive a branch called v1.1 that forks v1.0 and send a pull request from master to that branch so I can make editorial comments?

In assimilation, we should note that then must be called in a separate event to prevent plan interference as mentioned by @erights. We need specs that deliberately attempt to interfere.

As a matter of copy, all occurrences of “which” that can be changed to “that” and have the intended meaning should probably be changed to “that”. We are free and loose with “which” colloquially.

domenic commented 11 years ago

Can we contrive a branch called v1.1 that forks v1.0 and send a pull request from master to that branch so I can make editorial comments?

Does this work?

In assimilation, we should note that then must be called in a separate event to prevent plan interference as mentioned by @erights. We need specs that deliberately attempt to interfere.

I'm not sure that's necessary for the ways we use assimilation currently. (Happy to be proved wrong though.) If we specify assimilation generally, i.e. try to specify Q() or when(), then definitely we'd want that.

kriskowal commented 11 years ago

@domenic, yes and good point.

domenic commented 11 years ago

Both @kriskowal and @briancavalier have indirectly raised the point that I should really get cracking on a test suite for 1.1, so that we can be sure we're covering everything as precisely as we've hoped. I'll do that tonight.

domenic commented 11 years ago

Tests in progress, but there's a lot left to write for the assimilation procedure.

https://github.com/promises-aplus/promises-tests/tree/spec-1.1

I think it'd be best if I put this off until the weekend. In the meantime, would be great for others to chime in :).

domenic commented 11 years ago

I just merged the switchover to the Promise Resolution Procedure, as a result of discussions in #87 and #88. As such I'm resetting our ratification counter.

The tests still aren't finished but I hope to do so this weekend.

briancavalier commented 11 years ago

Thanks @domenic. I'll have time to give the updated spec a thorough read tomorrow.

ForbesLindesay commented 11 years ago

I'm still uneasy about resolution procedure 2.2:

If retrieving the property x.then results in a thrown exception e, reject promise with e as the reason.

I'd much prefer:

If retrieving the property x.then results in a thrown exception, resolve promise with x.

The reason for this is that I definitely view that as the correct behavior if x is in fact some kind of proxy object that just throws on access to all properties which are undefined. I could envisage someone doing that in an effort to tend towards "defensive programming" and it might reasonably be done with no awareness of the promises/A+ spec.

rkatic commented 11 years ago

if x is in fact some kind of proxy object that just throws on access to all properties which are undefined.

I am not sure how such proxy would be practical. Nearly every duck-type test in the wild would throw. Expecting that with es6 everyone will start to wrap property checks in try..catch blocks, is overoptimistic at least.

In case I am wrong, would an additional check with the in operator help?

domenic commented 11 years ago

@ForbesLindesay, I see your uneasiness and somewhat agree. The way I reconciled it is over in https://github.com/promises-aplus/promises-spec/issues/88#issuecomment-15153965.

In case I am wrong, would an additional check with the in operator help?

It's a tempting idea, but I don't think it's great, because it is observable by proxies (who could do weird things like return true for the HasProperty trap, but then give no value for the Get trap), and it doesn't match existing ES5 spec algorithms.

briancavalier commented 11 years ago

Thanks to @domenic for prodding me today via twitter :)

It seems like #87 and #88 were the last hold-ups for 1.1. The spec accounts for those oddities now, and the issues have been closed.

Paging all to review and sign off or raise any remaining issues ASAP.

briancavalier commented 11 years ago

@domenic, what's the status of the test suite for 1.1? Can we dive in and help?

domenic commented 11 years ago

@briancavalier It's like half-done. It's my top priority after HTML5DevConf (so after Tuesday).

domenic commented 11 years ago

The progress is at https://github.com/promises-aplus/promises-tests/tree/spec-1.1 BTW.

briancavalier commented 11 years ago

Great, thanks.

ForbesLindesay commented 11 years ago

Providing everyone else supports the current proposed behavior for throwing get accessors I'll accept it and remove my objection. Meaning it has my sign off.

wycats commented 11 years ago

Ratified.

erights commented 11 years ago

(I don't know how well github does with email quoted reply format. If not well, I'll try to reenter this again by other means.)

The then Method A promise must provide a then method to access its current or eventual fulfillment value or rejection reason. A promise's then method accepts two arguments: promise.then(onFulfilled, onRejected)

  1. Both onFulfilled and onRejected are optional arguments:
  2. If onFulfilled is not a function, it must be ignored.

I am uncomfortable saying that if it is not a function it must be ignored. I would prefer to say that if it is undefined, it must be ignored. If it is not a function nor undefined, that this causes the returned promise to be rejected with a TypeError. Likewise for onRejected.

  1. If onRejected is not a function, it must be ignored.
  2. If onFulfilled is a function:
  3. it must be called after promise is fulfilled, with promise's fulfillment value as its first argument.

I don't think this wording works for a far reference. A far reference is a fulfilled promise whose fulfillment value is remote, and therefore cannot itself be directly provided at the argument to the onFulfilled function. Rather, a far reference's then calls the onFulfilled function with itself (or an equivalent far reference) as argument.

The wording you've got does fit the "there" method < http://wiki.ecmascript.org/doku.php?id=strawman:concurrency#there>, but I doubt you want to deal with "there" at this time ;).

  1. then must return before onFulfilled or onRejected is called [4.1https://github.com/promises-aplus/promises-spec/blob/master/README.md#notes ].

As we discussed, the important invariant is that these are called from an empty stack. This implies "top of turn" but without saying too much about the turn mechanism. This "empty stack" constraint is compatible both with the macro-turn architecture of Q and the micro-turn (or end-of-task) architecture of DOMFuture.

  1. onFulfilled and onRejected must be called as functions (i.e. with no this value). [4.2https://github.com/promises-aplus/promises-spec/blob/master/README.md#notes ]
  2. then may be called multiple times on the same promise.

This will be a tricky bit of wording to get right, but it is worth noting that the above constraints such as "onFulfilled must not be called more than once" apply per call to then, not per identity of the onFulfilled function, which is how that constraint would be naively misread.

  1. If either onFulfilled or onRejected returns a value x, run the Promise Resolution Procedure Resolve(promise2, x).

Is it clear that the "Resolve" function mentioned here is a spec device and not an actual function? We need to avoid implying that there is an operation that can cause promise2 to become resolved given only promise2.

  1. If either onFulfilled or onRejected throws an exception e, promise2 must be rejected with e as the reason.
  2. If onFulfilled is not a function and promise1 is fulfilled, promise2 must be fulfilled with the same value.

I prefer "...is undefined and..."

If onFulfilled is neither a function nor undefined, promise2 must be rejected with a TypeError as the reason. Likewise for onRejected.

  1. If onRejected is not a function and promise1 is rejected, promise2 must be rejected with the same reason.

The Promise Resolution Procedure The promise resolution procedure is an abstract operation taking as input a promise and a value, which we denote asResolve(promise, x).

Ah. Answers my previous worry. But perhaps this needs to be stated earlier. Perhaps, following Ecma-262, this should be spelled "[[Resolve]]".

If x is a thenable, it attempts to make promise adopt the state of x, under the assumption that xbehaves at least somewhat like a promise. Otherwise, it fulfills promise with the value x. This treatment of thenables allows promise implementations to interoperate, as long as they expose a Promises/A+-compliant thenmethod. It also allows Promises/A+ implementations to "assimilate" nonconformant implementations with reasonable then methods. To run Resolve(promise, x), perform the following steps:

  1. If x is a promise, adopt its state [4.4https://github.com/promises-aplus/promises-spec/blob/master/README.md#notes ]:
  2. If x is pending, promise must remain pending until x is fulfilled or rejected.
  3. If/when x is fulfilled, fulfill promise the same value.
  4. If/when x is rejected, reject promise with the same reason.
  5. Otherwise, if x is an object or function,
  6. Let then be x.then. [4.5https://github.com/promises-aplus/promises-spec/blob/master/README.md#notes ]
  7. If retrieving the property x.then results in a thrown exception e, reject promise with e as the reason.
  8. If then is a function, call it with x as this, first argument resolvePromise, and second argument rejectPromise, where:
  9. If/when resolvePromise is called with a value y, run Resolve(promise, y).
  10. If/when rejectPromise is called with a reason r, reject promise with r.
  11. If both resolvePromise and rejectPromise are called, or multiple calls to the same argument are made, the first call takes precedence, and any further calls are ignored.
  12. If calling then throws an exception e,
  13. If resolvePromise or rejectPromise have been called, ignore it.
  14. Otherwise, reject promise with e as the reason.
  15. If then is not a function, fulfill promise with x.
  16. If x is not an object or function, fulfill promise with x.

Must put this last case prior to thenable checking. It doesn't matter if it comes before or after promise checking since those cases are disjoint.

The reason the primitive value check must come before the thenable check is that these cases are not disjoint. For example,

Boolean.prototype.then = ....;
var p2 = p1.then(_ => true);

must fulfill p2 with true, even though true would pass the thenable check taken by itself. The primitive and the promise checks must both happen prior to the thenable check because an explicit lifting step such as Q(v) should do the primitive and promise checks synchronously, but must postpone the thenable check to avoid plan interference hazards.

Notes

  1. In practical terms, an implementation must use a mechanism such as setTimeout, setImmediate, or process.nextTick to ensure that onFulfilled and onRejected are not invoked in the same turn of the event loop as the call to then to which they are passed.

Those take case of the macro-turn case. As long as we're mentioning these explicitly, we may as well mention that micro-turn systems like DOMFuture can conform to Promises/A+, and can use mechanisms such as Object.observe to do the micro-turn scheduling.

This will be worth a big discussion somewhere, but the more I think about it, the more important assimilation becomes. In particular, we need promises for both micro-turns and macro-turns, and assimilation allows these to co-exist cleanly.

  1. That is, in strict mode this will be undefined inside of them; in sloppy mode, it will be the global object. Implementations may allow promise2 === promise1, provided the implementation meets all requirements. Each implementation should document whether it can produce promise2 === promise1 and under what conditions.

I don't see how this could ever be possible under the constraints of the rest of this spec. Could you give an example?

  1. Generally, it will only be known that x is a true promise if it comes from the current implementation. This clause allows the use of implementation-specific means to adopt the state of known-conformant promises.
  2. This procedure of first storing a reference to x.then, then testing that reference, and then calling that reference, avoids multiple accesses to the x.then property. Such precautions are important for ensuring consistency in the face of an accessor property, whose value could change between retrievals.

https://github.com/promises-aplus/promises-spec/blob/master/README.md#generalhttps://github.com/promises-aplus/promises-spec/blob/master/README.md#terminologyhttps://github.com/promises-aplus/promises-spec/blob/master/README.md#requirementshttps://github.com/promises-aplus/promises-spec/blob/master/README.md#promise-stateshttps://github.com/promises-aplus/promises-spec/blob/master/README.md#the-then-methodhttps://github.com/promises-aplus/promises-spec/blob/master/README.md#the-promise-resolution-procedurehttps://github.com/promises-aplus/promises-spec/blob/master/README.md#notes

1.

erights commented 11 years ago

It looks like github did well enough. The only issue I spot in the above text is that the first text line of some of my replies inappropriately appear as if they are part of the previously quoted text. Knowing that, the above text is easy enough to read.

ForbesLindesay commented 11 years ago

I would suggest that you may want to summarize the text if it actually matters that people know this stuff. You only have to write it once, but I imagine there are at least 10 people who have to read this. People quoting entire e-mails is the bane of ES-Discuss, lets not make it a problem here too.

erights commented 11 years ago

Ok.

novemberborn commented 11 years ago

On footnote 2, I don't see how promise1 could ever be === promise2. I would appreciate an example of how this could possibly occur within an implementation conforming to the rest of promises/A+.

(I think you mean footnote 3)

Legendary has this behavior when no callbacks are passed:

promise1.then() === promise1;
erights commented 11 years ago

Gotcha. I hadn't considered the no-callbacks case.

ForbesLindesay commented 11 years ago

far promises may call onFulfilled with themselves

Couldn't such Far-Promises just not have a .then method?

The invariant provided by the event-loop model is that these callbacks are only called when the stack is empty

No, the important invariant the event loop turn is trying to create is that ordering of code is at least moderately consistent. By which we mean, always asynchronous. This doesn't always mean a completely empty stack, as libraries can optimize by providing an internal representation of the stack. The stack has to empty between the .then callback being added and the onFulfilled method being called, but it doesn't need to still be empty when the onFulfilled method is called.

Resolve should be called "[[Resolve]]"

I agree, spelling it [[Resolve]] sounds like a good idea.

A value which is both must be treated as a primitive value rather than a thenable.

Isn't that what the spec currently does

erights commented 11 years ago

A far reference is a promise and must have a then method. Otherwise, the promise patterns using the then method wouldn't work at all when encountering a far reference.

These are both important invariants. That the ordering invariants are important does not contradict that the empty stack invariant is important.

Boolean.prototype.then = function(){}; var p2 = p1,then(function() { return true; });

As I read the current spec, it will treat the above true as a thenable. It must treat it as a primitive value, fulfilling p2 with true.

rkatic commented 11 years ago

I am not too comfortable with allowing:

promise1.then() === promise1;

becouse

promise1.then(x => x) !== promise1;

Consistency is, in my opinion, much important then a minor and doubtful gain in performances. Knowing that then always returns a new promise could be important for security reasons (changes on a new promise will have no effects on the first one).

ForbesLindesay commented 11 years ago

@erights

None of the stuff in this spec does anything with far references, it only handles the .then method, which doesn't do anything meaningful with far references anyway. I have no problem with such far-references being thenables but not Promises/A+ Promises.

No, the empty stack was purposefully not required. It doesn't add anything on its own, it was just one way of trying to write that you can't call the function synchronously without saying "in the next tick". It was removed as a requirement because it turns out to prevent some very important optimizations that have no undesirable side effects.

No, it won't treat the true as a thenable. Read "The Promise Resolution Procedure" more carefully. Point 1 doesn't apply because it's not a true "promise". Point 2 doesn't apply because that's only if x is an object or function (true is neither). Point 3 does apply, so p2 in your example is fulfilled with true as its value.

@rkatic

There are two issues with that argument.

  1. There isn't any security gain yet because so far we've always considered promises to have no way of being modified if you only have the promise itself (you need some other object).
  2. We are considering adding a way for changes to be made to a promise when you only have the promise via the cancellation API, but these will propagate up the chain of .then calls anyway. You are looking for a new primitive such as the proposed .fork if you want to create a securely separated promise.
rkatic commented 11 years ago

@ForbesLindesay are you saying that usage of Object.freeze is considered mandatory to prevent promises to be modified?

erights commented 11 years ago

@ForbesLindesay You write: "I have no problem with such far-references being thenables but not Promises/A+ Promises." Well I do. I would like Q to conform to promises/A+ but be able to recognize if own far references as promises within the resolution procedure.

I understand that nothing in the promises/A+ spec is about far references. I am not suggesting otherwise. I am simply saying that the promises/A+ spec should not be incompatible with far references.

On the empty stack, please give an example of an optimization which is possible otherwise, and with "no undesirable side effects". An example would help me understand what you have in mind.

On the order of checking and my true example, you are exactly right. Somehow I missed the "if x is an object or function" part at the beginning of point 2. Thanks.

ForbesLindesay commented 11 years ago

@rkatic

No, but there's equally nothing preventing you from doing something like:

var oldThen = Promise.prototype.then;
Promise.prototype.then = function (cb, eb) {
  var res = oldThen.apply(this, arguments);
  res.parent = this; //this is allowed, and breaks your requirement anyway
  return res;
};

The result would still be fully Promises/A+ compliant.

@erights

Which statement of the spec is (in your view) incompatible with interoperability between promises and far references. The Q library hasn't had any issues meeting the spec thus far and I think @kriskowal (or @domenic) should jump in if they feel this might raise some issues. I don't think the spec actually causes any problems with regards to interop with far-references.

As for the optimization, instead of calling next tick for each promise, you create an internal queue of promises that must be run in the next tick. You then empty this queue once the only thing in the stack is your own library, because at that point, the outside world can't tell you haven't gone to the next tick, other than the fact that if you do this forever you'll starve the IO. I don't want to go into tonnes of detail on this, but it's been discussed at some length in previous issues both here and in Q (where I think there was some talk of trying this out).

rkatic commented 11 years ago

@ForbesLindesay altering the prototype is clearly an abuse, but altering an instance could be considered fine for a user that believes that he owns that object. Maybe I am too paranoid..

ForbesLindesay commented 11 years ago

The code I listed above could go inside a promise library and still be legal, that was the point I was attempting to make:

amazing-promises.js:


funciton Promise(fn) {
  // magically create `resolve`, `reject`
  fn(resolve, reject);
}

Promise.prototype.then = function (cb, eb) {
  //magically use `cb` and `eb` apropriately and get the result
  //which is promise like and call it `res`
  res.parent = this; //this is allowed, and breaks your requirement anyway
  return res;
};

Assuming you implemented the bits I've labelled as magic, the above would be a promises/A+ compatible library.

erights commented 11 years ago

@ForbesLindesay writes "Which statement of the spec is (in your view) incompatible with interoperability between promises and far references."

I think it's only a problem of wording, not a deep issue. The statement I object to is "If onFulfilled is a function: it must be called after promise is fulfilled, with promise's fulfillment value as its first argument." The issue is that a far reference's fulfillment value is remote, and therefore cannot be used in a local call to the onFulfilled function.

If the wording of Promises/A+ were not changed in this regard, it wouldn't be a disaster. It would just push the awkward phrasing issue into the description of the Q library. Q would need to say that a far reference's fulfillment value is itself.

The optimization you have in mind seems fine and is one I encourage. I don't see how it violates the empty stack constraint. The loop you're using to empty this queue is part of the implementation of the JS-enhanced-with-promises platform. No activation frames within user code are stacked up at the beginning of each iteration of your loop. Each iteration of that loop begins in an empty stack.

rkatic commented 11 years ago

@ForbesLindesay to prevent more repeating, I will stop here. Subject not particularly important anyway.

domenic commented 11 years ago

OK, back from HTML5DevConf! Ready to dig into this. @erights:

Rather than case splitting onFulfilled and onRejected on whether they are a function or not, case split three ways: a) whether they are a function, b) undefined -- treat as the current non-function case, c) neither a function nor undefined -- treat as equivalent to throwing a TypeError, i.e., rejecting the returned promise with a TypeError reason.

We discussed this in #25, with the conclusion that this approach is not acceptable, as it will break too much promise code in the wild that uses null in place of undefined (and even some that uses false). For what it's worth, there are examples in the ES5 spec of both behaviors: JSON.parse and JSON.stringify ignore non-[[Callable]]s, whereas the Array.prototype methods throw TypeErrors.

Find a wording for the onFulfilled case that works for far references, where the fulfillment value is remote and therefore cannot be provided as argument to the onFulfilled function. In this case the far reference provides itself as the argument. I am not suggesting that the spec say anything specifically about far references or this behavior, but it must allow this behavior.

This is a tricky one. Suggestions welcome.

The most important invariant provided by the turn/event-loop model is that these callbacks are only called when the stack is empty. The current spec seems only accidentally weaker than that and should be fixed.

I tried to get this in, on your suggestion, in #70. But there were concerns about clarity and about the user-space trampoline argument discussed above; see especially this explanation from @briancavalier. In particular, capturing the subtlety of

The loop you're using to empty this queue is part of the implementation of the JS-enhanced-with-promises platform.

is tricky. What would the phrasing be? "When the function execution stack is empty, except that it may also contain code from the promise implementation"? Suggestions definitely welcome.

Since "Resolved" is not a JS function but only an internal spec device, it should be spelled "[[Resolve]]".

Will do!

briancavalier commented 11 years ago

In particular, capturing the subtlety of

The loop you're using to empty this queue is part of the implementation of the JS-enhanced-with-promises platform.

is tricky. What would the phrasing be? "When the function execution stack is empty, except that it may also contain code from the promise implementation"? Suggestions definitely welcome.

I'm all for trying to find some language for this. It seems very tricky without re-defining "call stack", or somehow specifically classifying promise implementation code as part of the "platform", as @erights suggested. Or maybe something along the lines of what @domenic is suggesting, but how to phrase it nicely is stymieing me .. maybe something about only promise implementation code being allowed below onFulfilled/onRejected on the call stack?

Since "Resolved" is not a JS function but only an internal spec device, it should be spelled "[[Resolve]]".

+1

If the wording of Promises/A+ were not changed in this regard, it wouldn't be a disaster. It would just push the awkward phrasing issue into the description of the Q library. Q would need to say that a far reference's fulfillment value is itself.

I understand the issue with remotes. My worry here, though, is that if we aren't careful, we may open the door to naive implementations doing weird things, like leaking promises into handlers when it really isn't necessary or desirable. The only thought I have right now is to somehow say that if the fulfillment value has become available but a variable/binding (whatever the correct ES term is here?) can't be provided for it, a promise for it must be provided instead. That'll def cause some head scratching :/

domenic commented 11 years ago

Tests are updated!

https://github.com/promises-aplus/promises-tests/compare/spec-1.1

When.js passes with flying colors, RSVP and Q fall down on a few points.

novemberborn commented 11 years ago

Legendary passes too (under Node v0.10), so I'm +1 ;-)

domenic commented 11 years ago

I think 1.1 is ready :). Going to try pushing a new version to gh-pages, and thus to http://promisesaplus.com/

juandopazo commented 11 years ago

Sounds good. Some of us will probably just start looking at ES6-AP2 which is consistent with 1.1.