Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Fix spec #5

Merged
merged 3 commits into from
Aug 1, 2016
Merged

Fix spec #5

merged 3 commits into from
Aug 1, 2016

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented May 24, 2016

Attempt to fix #2 and close #1.

Spec HTML viewable here

This attempts to fix the spec by:

  1. spec-refactor of Promise.resolve logic so the meat of it is available as an abstract operation
  2. use that abstract operation to produce a promise with certainty inside PromiseReactionJob, and resolve that with a thunk for the intended "argument".

@ljharb ljharb force-pushed the fix_spec branch 2 times, most recently from 09be010 to ecbbf91 Compare June 6, 2016 23:27
@ljharb ljharb force-pushed the fix_spec branch 2 times, most recently from ad71038 to e325825 Compare July 21, 2016 17:44
@ljharb
Copy link
Member Author

ljharb commented Jul 24, 2016

@domenic any thoughts on this solution?

1. let _handlerResult_ be Call(_handler_, *undefined*, « »).
1. If _handlerResult_ is not an abrupt completion, then
1. Let _handlerPromise_ be ? PromiseResolve(%Promise%, _handlerResult_.[[Value]]).
1. Let _argumentThunk_ be equivalent to a function that returns _argument_.

Choose a reason for hiding this comment

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

neat, but can this be described like this without it being considered a layering violation? (I honestly don't know) @domenic ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This text is identical to text that used to be in the spec prior to tc39/ecma262#584 so I'm pretty sure it's OK. Not sure what you mean by "layering violation" tho?

Copy link

@stefanpenner stefanpenner Jul 29, 2016

Choose a reason for hiding this comment

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

Seems good then, it just seemed like it was describing language semantics with those semantics. But if its based on prior art we are likely good.

@@ -356,7 +356,7 @@ <h1 class="title">Promise.prototype.finally</h1>
</emu-alg>
</emu-clause>

<!-- es6num="25.4.1.8" -->
<!-- es6num="25.4.4.5" -->
Copy link
Member Author

Choose a reason for hiding this comment

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

@bterlson this seems to have changed between when I initially wrote this PR and now. Was this intentional? I thought "es6num" values were supposed to stay constant forever?

Copy link
Member

Choose a reason for hiding this comment

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

It's 25.4.4.5 in the spec now, so presumably this change is fine. Not sure where 25.4.1.8 came from. It should remain unchanged from ES6.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unless I copy-paste-errored, 25.4.1.8 is what it was when I first copied the spec steps for Promise.resolve and started writing this PR. Hopefully it's unchanged and I just messed it up :-)

@ljharb ljharb merged commit 335de82 into master Aug 1, 2016
@ljharb ljharb deleted the fix_spec branch August 1, 2016 04:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix spec text
3 participants