promises-aplus / synchronous-inspection-spec

An extension to the Promises/A+ spec for synchronously retrieving fulfillment values and rejection reasons
5 stars 1 forks source link

Draft E #6

Open ForbesLindesay opened 11 years ago

ForbesLindesay commented 11 years ago

Promises/A+ Extension: Synchronous Inspection

This proposal extends the Promises/A+ specification to cover synchronous inspection of a promise's fulfillment value or rejection reason.

It is not expected that all Promises/A+ implementations will include this extension. If the features of this extension are desired, you should test for them:

if (typeof promise.inspectState === "function") {
  // Use the `inspectState` method, assuming it conforms to the contract below.
}

Motivation

TODO

Requirements: the inspectState method

A promise implementing this specification must have an inspectState method, which returns an object.

  1. When the promise is pending, the object returned by inspectState
    1. must have a property named isFulfilled with a value of false
    2. must have a property named isRejected with a value of false
    3. must not have a property named fulfillmentValue
    4. must not have a property named rejectionReason
  2. When the promise is fulfilled, the object returned by inspectState
    1. must have a property named fulfillmentValue, whose value is equal to the promise's fulfillment value.
    2. must have a property named isFulfilled with a value of true
    3. must have a property named isRejected with a value of false
    4. must not have a property named rejectionReason
  3. When the promise is rejected, the object returned by inspectState
    1. must have a property named rejectionReason, whose value is equal to the promise's rejection reason.
    2. must have a property named isFulfilled with a value of false
    3. must have a property named isRejected with a value of true
    4. must not have a property named fulfillmentValue
  4. Adding to or modifying properties of the object returned by inspectState must not impact the state, fulfillment value, or rejection reason of the promise in any way. (note 2)

    Notes

  5. Since fulfillment values and rejection reasons can be any valid JavaScript value, including undefined, you must check isFulfilled and isRejected. That is, the proper way to synchronously test for a promise being fulfilled is not if (promise.inspectState().fulfillmentValue) or even if (promise.inspectState().fulfillmentValue !== undefined), but instead if (promise.inspectState().isFulfilled).
  6. Implementations may, if they choose, freeze the returned object in order to make this more obvious.
ForbesLindesay commented 11 years ago

This spec is very similar to Draft B but I think people are slightly less likely to get the check for isFulfilled wrong with an explicit property.

domenic commented 11 years ago

This is probably a good idea. Might be nice to add isPending and keep the specification that the other properties don't exist.

ForbesLindesay commented 11 years ago

I think the restriction of the other properties not existing seems un-necessary providing you have the boolean values to check.

I didn't add isPending because I was trying to keep it as minimal as possible while avoiding awkward concepts like checking properties for existence.

I'm flexible on both those points though. Let's see if anyone else has thoughts on those two points.

domenic commented 11 years ago

I think the restriction of the other properties not existing seems un-necessary providing you have the boolean values to check.

Well, remember that we're writing a specification here ;). Anything left unspecified can be used for great evil, e.g. rejectedPromise.inspectState().fulfillmentValue = "a very legitimate value".

ForbesLindesay commented 11 years ago

Actually, I was thinking of performance implications of adding properties being potentially significant, but we don't need to worry about that as we should really return a fresh object each time.

ForbesLindesay commented 11 years ago

OK, I've added those in. I'm still yet to be convinced of the value added by an extra isPending property being specc'd.

domenic commented 11 years ago

One last thing: I think we need language that states that modifying the object returned from inspectState should not impact the state of the promise. I'll edit your OP to include that; hope you don't mind.

Otherwise, I think this is great and we should push for consensus.

domenic commented 11 years ago

Gaah, wait, I'm not an admin here, I can't edit. Anyway, I'm thinking a requirement 4, along the lines of

  1. Adding to or modifying properties of the object returned by inspectState must not impact the state, fulfillment value, or rejection reason of the promise in any way. (note 2)

(note 2): implementations may, if they choose, freeze the returned object in order to make this more obvious.

novemberborn commented 11 years ago

+1

domenic commented 11 years ago

What do we think of a state property that can be either "fulfilled", "rejected", or "pending" instead of the isFulfilled and isRejected booleans?

novemberborn commented 11 years ago

isFulfilled seems shorter:

if(promise.inspectState().isFulfilled){

versus

if(promise.inspectState().state === "fulfilled"){
ForbesLindesay commented 11 years ago

I've added requirement 4 as per @domenic's suggestion

The booleans seem less prone to typos like:

if(promise.inspectState().state === "fulfiled"){
  //never happens
}
domenic commented 11 years ago

@ForbesLindesay why is that typo more likely than

if (promise.inspectState().isFulfiled) {
  // never happens
}

?

domenic commented 11 years ago

@kriskowal would something like this work for your Montage use case, or would the non-live object returned by inspectState be a non-starter?

kriskowal commented 11 years ago

Yeah, non-reactive state object would not fly for the Montage use-case if you wanted to use a promise as a direct controller for bindings. Bear in mind that such a system would be convenient but not necessary since it’s trivial to construct a reactive state object with then.

https://github.com/montagejs/montage/blob/integrate-bindings/core/promise-controller.js

domenic commented 11 years ago

Oh right, that makes sense. So this wouldn't be any better or worse than what Q already does for that use case.

General comments welcome while you're here :).

kriskowal commented 11 years ago

I would favor isFulfilled(), isRejected(), isPending(), getValue(), and getError() methods directly on the promise, or, if we restrict ourselves to ES5, value and error accessors directly on the promise. We can’t get away without a function call in those cases because the value and error properties necessarily come from the most resolved promise in the forwarding chain. A Stat snapshot object is overkill.

domenic commented 11 years ago

I guess the desire was to minimize API surface and make it easily testable if you're dealing with a promise implementation that has these features (typeof promise.inspectState === "function"). Plus the snapshot idea avoids ES5 vs. ES3 concerns.

kriskowal commented 11 years ago

@domenic I’m not buying it from a user perspective. At the very least, fulfilledValue should be value and rejectionReason should be error. There are no other meaningful uses of value or errror in those contexts so there is no chance of confusion.

domenic commented 11 years ago

You're always so insistent on "error" over "reason," heh ;). If anything "exception" seems better since it's not necessarily instanceof Error... hmm...

ForbesLindesay commented 11 years ago

I think:

if (promise.inspectState().isFulfiled) {
  // never happens
}

is a marginally less likely typo because clever auto complete/static analysis might be expected to pick that up and make suggestions, but I haven't yet seen one that attempts to fix text you put in string literals.

I like value and reason. I'm not sure about error or exception since we haven't used those words anywhere else to talk about promise states.

novemberborn commented 11 years ago

FYI I've implemented this in legendary@0.8.0, with value and reason instead of fulfillmentValue and rejectionReason.

@ForbesLindesay remind me why we require both isFulfilled and isRejected?

ForbesLindesay commented 11 years ago

Because it could be pending, fulfilled, or rejected. To store 3 possible states you require 2 bits. I'm treating each property as a boolean (not some kind of tri-state with null as an extra option).

novemberborn commented 11 years ago

Sorry, I wasn't clear enough. Why do we require both isFulfilled and isRejected when the promise is fulfilled or rejected? isPending === (!isFulfilled && !isRejected), but isFulfilled === isFulfilled && !isRejected shouldn't be necessary.

ForbesLindesay commented 11 years ago

There's no isPending in the spec I wrote. You must have at least two of the three states be specified, then you can work out the third easily enough. Requiring them to be false seems much cleaner than requiring them to be undefined, and we can't leave them unspecified because then someone could choose to return {isFulfilled: true, isRejected: true} which isn't a valid promise state.

novemberborn commented 11 years ago

Requiring them to be false seems much cleaner than requiring them to be undefined

Cleaner how? You already write "Must not have a property named reason", so why not add "Must not have a property named isFulfilled".

ForbesLindesay commented 11 years ago

Because the normal check runs:

var state = promise.inspectState();
if (state.isFulfilled) {
  //do something
}

Feels a lot more natural if isFulfilled is equal to true or false than if it's equal to true or undefined. This is even more true in the case of isPending = !(isFulfilled || isRejected)

domenic commented 11 years ago

I am really starting to dislike the booleans approach, favoring instead a single state value that is always one of the strings "pending", "fulfilled", or "rejected". I didn't see an answer to my earlier question that would dissuade me from that.

novemberborn commented 11 years ago

Booleans do allow for extra information, e.g. I'd like to encode whether the promise was cancelled, without having to inspect the reason.

ForbesLindesay commented 11 years ago

I'm fine with either, I just prefer testing an object for known values, than testing for the existence/lack of existence of a property.