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

New wording for thenable resolution #124

Closed
wants to merge 1 commit into from

Conversation

lsmith
Copy link
Contributor

@lsmith lsmith commented Jun 4, 2013

The v1.1 language for the Promise Resolution Procedure dictates steps for an
implementation rather than the rules that an implementation must follow. The
rest of the spec is in the form of a set of rules, leaving the implementation
details up to the implementer. Whatever sausage is under the hood, as long as
the test suite is passed, it is a conformant implementation.

This attempts to normalize the language into the set-of-rules style.

The v1.1 language for the Promise Resolution Procedure dictates steps for an
implementation rather than the rules that an implementation must follow. The
rest of the spec is in the form of a set of rules, leaving the implementation
details up to the implementer. Whatever sausage is under the hood, as long as
the test suite is past, it is a conformant implementation.

This attempts to normalize the language into the set-of-rules style.
@juandopazo
Copy link
Contributor

👍 I like specs with algorithms, but this makes more sense for A+ and the community.

@briancavalier
Copy link
Member

I think one of the most interesting things about Promises/A+ is that the spec encourages innovation since we haven't locked everything down to the exact algorithm level. For that reason, I'm glad to see @lsmith take a shot at this. There are some tricky bits in this algorithm, though, so I'm def interested to give it a thorough read. Should have time over the next day or two.

@briancavalier
Copy link
Member

Left a few little comments, but I want to read the two approaches together to make sure they do cover the same cases. They are so different that it's tough to compare :)

1. If either callback returns a value which is an object or function with a `then` method, but the value cannot be identified as a promise (call the value `thenable`) attempt to fulfill or reject `promise2` via `thenable`'s `then` method[[4.6](#notes)].

```js
thenable.then(resolver, rejecter);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these names are a bit confusing, especially resolver. I'd prefer the more verbose resolvePromise2 and rejectPromise2.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From DOM Promises: resolveCallback and rejectCallback

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like resolvePromise2 and rejectPromise2 because they are both explicit and contrived. It would be difficult for someone reading the spec to think that the names might have a more general connotation, or might be found elsewhere in the spec.

  • Rename resolver and rejecter to resolvePromise2 and rejectPromise2

@domenic
Copy link
Member

domenic commented Jun 5, 2013

I am hesitantly in favor of this. There are two major weaknesses I currently see:

  • It loses in precision and strength of wording in treating the throwing-then-getter case. It relegates that stuff to a footnote, which has traditionally meant clarification and not mandated behavior.
  • It will be harder to refactor this algorithm to use with a potential promise constructor spec, as in Strawman: Promise Creation API/D constructor-spec#18

@domenic
Copy link
Member

domenic commented Jun 5, 2013

I also think that the current style of stating requirements for then, but saying that one of those requirements is to run a very specific procedure when a certain event occurs, is not that bad. It might be clearer from an implementer point of view.

@briancavalier
Copy link
Member

@domenic could you explain your second bullet point ("harder to refactor ... with a ... promise constructor")? That sounds important, but I didn't understand what you're getting at.


Due to the recursive nature of this procedure, it is possible for a conformant implementation to cause infinite recursion if a promise is resolved with a thenable that participates in a circular thenable chain. Implementations are allowed, but not required, to detect such occurrences and instead reject `promise` with an informative `TypeError` as the reason.
1. If either callback (`onFulfilled` or `onRejected`) throws an exception, `promise2` must be rejected with the thrown exception as the reason.
1. If either callback returns a value that is not an object or function with a `then` method[[4.5](#notes)], `promise2` must be fulfilled with that value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the way to arrange this might be:

  1. if they throw, reject
  2. if they return promise2, reject with TypeError
  3. if they return a promise, adopt state
  4. otherwise, let then be returnValue.then,
    1. if this step throws, reject with thrown error
    2. if then is a function, call it, etc.
    3. otherwise, fulfill with returnValue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had almost that exact arrangement in a working draft, but went back from "otherwise" to "if" because it would be the only use of the word in the spec and, for me at least, began mirroring if/else blocks which suggested code structure. Also, the nesting started to get a little deep (the irony). The bullets are exclusive of one another, so "otherwise" does makes sense. Other opinions?

The point about accessing then only once being in the spec vs in the notes I'm on the fence about. Like thenable.then(resolvePromise2, rejectPromise2) it is an implementation detail, not a rule, technically. The justification for it is good, albeit edge-casey. I'm willing to go with whichever placement is deemed best.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "otherwise" works well in this case, and I like that @domenic's ordering groups the two "outer" error conditions together as 1 and 2**. I'm also on the fence about explicitly stating that accessing thenable.then once is a requirement. However, it seems possible to write the steps in such a way that, when followed closely, you'd end up only accessing it only once anyway.

EDIT: **See my other note about step 2 not being the error condition we care about, tho :)

@domenic
Copy link
Member

domenic commented Jun 5, 2013

@briancavalier I meant that the promise constructor spec linked is currently very nice and simple: it states:

Calling resolve(x)

  1. If promise is resolved, nothing happens (in particular, no exception may be thrown).
  2. Otherwise, run the Promise Resolution Procedure [[Resolve]](promise, x).

But if we eliminated [[Resolve]] in favor of something that's incorporated into the main then specification, we'd lose the ability to simply delegate back to the main spec from the constructor spec, instead having to repeat ourselves.

@juandopazo
Copy link
Contributor

@domenic "A Resolve operation must satisfy the following requirements:`

@briancavalier
Copy link
Member

@domenic re: Promise constructor referencing [[Resolve]]. Got it, thanks for clarifying. Yeah, have to think about how to handle that.

@domenic
Copy link
Member

domenic commented Aug 20, 2013

I am no longer really in favor of this. It hasn't kept up with recent changes, and has unaddressed concerns. I like the precision of our current resolution procedure; it seems like exactly the right place to inject that precision, at a very tricky point in the process with lots of edge cases. Plus, uh, I already wrote all the tests for it in that form :P

Does anyone else want to champion this? Otherwise, I'd like to close it out, and call 1.1 done ( 💪 !)

@lsmith
Copy link
Contributor Author

lsmith commented Aug 20, 2013

I'm fine closing this. I haven't prioritized keeping up or keeping the PR up with the current state of Promises.

It does seem though that the A+ spec has transitioned from an implementation target for JS libs to a reference document/discussion platform for implementation in ES and DOM. If it is serving the role of guidance for soon-to-be JS polyfill implementations, that's fine, and should be accordingly specific.

If my assessment isn't off base, I consider that change a very good thing. However, if that is the case, do you think it would be advantageous to push a 1.1 when the state of native promises is (seemingly) close to resolution? Or is there already accepted agreement in ES and DOM for the behavior of then()?

@domenic
Copy link
Member

domenic commented Aug 20, 2013

It does seem though that the A+ spec has transitioned from an implementation target for JS libs to a reference document/discussion platform for implementation in ES and DOM.

Hmm, well, to some extent I think this is true. But Promises/A+ is also still focused, IMO primarily, on its original mission, which is interoperable promise implementation standards as driven by implementers. The DOM folks and TC39 are suffering from some non-implementer consensus-blockers that bloat their standards and drive them in unfortunate directions. Any user-space implementations by us, the people who have been doing this for a while, are unlikely to adopt such unfortunate antifeatures. So there's still a place for a community-driven standard. We also have different restrictions than they do, e.g. inter-library compatibility (see assimilation of thenables) or legacy compatibility (see ignore-vs-reject for non-undefined-non-function arguments to then). That said, I am inspired by the extent to which we have influenced those august bodies, and think it bodes well for future extensible web work.

(All of the above is my opinion, to be clear, and not necessarily reflective of everyone in Promises/A+-land.)

If it is serving the role of guidance for soon-to-be JS polyfill implementations, that's fine, and should be accordingly specific.

This is a kind of funny sentence, because the difference between polyfill implementations and just plain user-space promise implementations seems to come down to which standard (DOM/TC39 or Promises/A+) you are implementing. Heh :)

If my assessment isn't off base, I consider that change a very good thing. However, if that is the case, do you think it would be advantageous to push a 1.1 when the state of native promises is (seemingly) close to resolution? Or is there already accepted agreement in ES and DOM for the behavior of then()?

I definitely think it would be. Native promises are still in some flux, and even if they do settle down, it might not be in an exemplary direction. The DOM spec has notable bugs and divergences, and TC39 promises are currently a Lovecraftian mismash of monads and promises into the same object. Providing a clear example of where the community wants to go and what it considers important, in the form of Promises/A+ 1.1, seems like a worth goal. We still have time to influence these things substantially before they start shipping, without flags, as part of public APIs, in more than one major browser. Even if we don't influence them, I still believe promise libraries will have a place in the native-promise future, whether it be augmenting native promises with lots of methods or prototyping cool new features like unhandled rejection tracking, cancellation, synchronous state inspection, or generator integration.

So to reiterate, I think continuing as we are is good. I and others will of course continue to do outreach to the more official standards bodies, but having Promises/A+ 1.1 as a product of our community-driven open spec development process is worthwhile, both for that outreach and inherently toward our original goals.

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

Successfully merging this pull request may close these issues.

4 participants