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

Draft E #6

Open
ForbesLindesay opened this issue Feb 8, 2013 · 30 comments
Open

Draft E #6

ForbesLindesay opened this issue Feb 8, 2013 · 30 comments

Comments

@ForbesLindesay
Copy link
Member

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

  1. 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).
  2. Implementations may, if they choose, freeze the returned object in order to make this more obvious.
@ForbesLindesay
Copy link
Member Author

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
Copy link
Member

domenic commented Feb 8, 2013

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

@ForbesLindesay
Copy link
Member Author

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
Copy link
Member

domenic commented Feb 8, 2013

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

domenic commented Feb 16, 2013

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
Copy link
Member

domenic commented Feb 16, 2013

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
Copy link

+1

@domenic
Copy link
Member

domenic commented Feb 17, 2013

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

@novemberborn
Copy link

isFulfilled seems shorter:

if(promise.inspectState().isFulfilled){

versus

if(promise.inspectState().state === "fulfilled"){

@ForbesLindesay
Copy link
Member Author

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
Copy link
Member

domenic commented Feb 28, 2013

@ForbesLindesay why is that typo more likely than

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

?

@domenic
Copy link
Member

domenic commented Feb 28, 2013

@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
Copy link
Member

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
Copy link
Member

domenic commented Feb 28, 2013

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
Copy link
Member

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
Copy link
Member

domenic commented Feb 28, 2013

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
Copy link
Member

@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
Copy link
Member

domenic commented Feb 28, 2013

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

@ForbesLindesay
Copy link
Member Author

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
Copy link

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
Copy link
Member Author

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
Copy link

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
Copy link
Member Author

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
Copy link

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
Copy link
Member Author

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
Copy link
Member

domenic commented Apr 1, 2013

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
Copy link

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
Copy link
Member Author

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.

domenic referenced this issue in promises-aplus/promises-spec Jun 3, 2013
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

4 participants