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

Deduplicate thenable coercion, and bring back thenable coercions weak map #62

Closed
domenic opened this issue Oct 28, 2013 · 0 comments
Closed

Comments

@domenic
Copy link
Owner

domenic commented Oct 28, 2013

The code of ThenableToPromise and Promise Reaction Functions both does a form of thenable coercion:

  • The former tries to coerce thenables straight-up into promises, so that its caller can use a trusted promise then or just pass the value on through.
  • The latter tries to send a "then" message to the return value of [[Handler]] using deferred.[[Resolve]] and deferred.[[Reject]] as the parameters, or just call deferred.[[Resolve]] if it's not a thenable.

These are pretty similar, but I believe I'm making them unnecessarily distinct in my current set up. E.g., if I moved some of the caller behavior into the former, the parallel would be clearer.

Once these functions are deduplicated, I will probably have an easier time reintroducing the thenable coercions weak map. I gave it a stab in bring-back-thenable-coercions branch but it did not pass the existing tests because I only did it inside ThenableToPromise and not inside Promise Reaction Functions.

I am reasonably happy with the verbiage surrounding the weak map in that branch, so the work here subsumes #39. Also I can try to be sure not to reintroduce any reentrance bugs, so let's subsume #61 in here too.

@ghost ghost assigned domenic Oct 28, 2013
domenic added a commit that referenced this issue Nov 17, 2013
domenic added a commit that referenced this issue Dec 7, 2013
This reverts commit bbc5b5c. As per #79, the thenable coercions weak map is, at least in its current form, not a good idea.

This also fixes #82, since the realm is no longer needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant