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

NewPromiseCapability & GetCapabilitiesExecutor Functions #392

Closed
raulsebastianmihaila opened this Issue Feb 14, 2016 · 10 comments

Comments

Projects
None yet
5 participants
@raulsebastianmihaila

raulsebastianmihaila commented Feb 14, 2016

The way GetCapabilitiesExecutor is used in NewPromiseCapability doesn't make sense to me.

First, there are some resolve and reject functions that are set as properties of the [[Capability]] slot of the executor that results from the operation. However, these resolve and reject functions are not passed as arguments in NewPromiseCapability. And, even if they were, in step 5 of NewPromiseCapability, the value of the [[Capability]] slot of the executor is replaced with the promise capability that was created in NewPromiseCapability, whose resolve and reject functions are undefined.

On the readability side, I think it would be an improvement to make GetCapabilitiesExecutor look more like other abstract operations: GetCapabilitiesExecutor(resolve, reject). And make it return F (the executor) instead of undefined. And explicitly pass resolve and reject from NewPormiseCapability.

@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

thefourtheye Feb 14, 2016

Contributor

Closely related to #355

Contributor

thefourtheye commented Feb 14, 2016

Closely related to #355

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Feb 14, 2016

Member

And as before, people just seem confused. GetCapabilitiesExecutor is not an abstract operation, and having it return something would be pointless, since nobody cares about its return value. The entire point of NewPromiseCapability is to turn a constructor C into a record of { {[Promise]], [[Resolve]], [[Reject]] }, i.e. a PromiseCapability. So having it take resolve and reject arguments would defeat its purpose. And the reason for setting the [[Capabilty]] slot of the executor is so that the executor can set its [[Capability]]'s [[Resolve]] and [[Reject]], so of course they start out undefined.

Member

domenic commented Feb 14, 2016

And as before, people just seem confused. GetCapabilitiesExecutor is not an abstract operation, and having it return something would be pointless, since nobody cares about its return value. The entire point of NewPromiseCapability is to turn a constructor C into a record of { {[Promise]], [[Resolve]], [[Reject]] }, i.e. a PromiseCapability. So having it take resolve and reject arguments would defeat its purpose. And the reason for setting the [[Capabilty]] slot of the executor is so that the executor can set its [[Capability]]'s [[Resolve]] and [[Reject]], so of course they start out undefined.

@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

thefourtheye Feb 14, 2016

Contributor

@domenic I believe that, it should be explained better in the spec. Every time I go through the spec I get confused at that point and looks like I am not the only one.

Contributor

thefourtheye commented Feb 14, 2016

@domenic I believe that, it should be explained better in the spec. Every time I go through the spec I get confused at that point and looks like I am not the only one.

@raulsebastianmihaila

This comment has been minimized.

Show comment
Hide comment
@raulsebastianmihaila

raulsebastianmihaila Feb 14, 2016

I see now. I think the most confusing part is that the... section... is called GetCapabilitiesExecutor Functions. My mind was telling me that it was a getter that was supposed to create an executor. But it's not. The executor is never created, it just is.

raulsebastianmihaila commented Feb 14, 2016

I see now. I think the most confusing part is that the... section... is called GetCapabilitiesExecutor Functions. My mind was telling me that it was a getter that was supposed to create an executor. But it's not. The executor is never created, it just is.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Feb 14, 2016

Member

It's created in step 4 of NewPromiseCapability.

Member

domenic commented Feb 14, 2016

It's created in step 4 of NewPromiseCapability.

@raulsebastianmihaila

This comment has been minimized.

Show comment
Hide comment
@raulsebastianmihaila

raulsebastianmihaila Feb 14, 2016

Yep, but not in GetCapabilitiesExecutor, as the name suggests.

raulsebastianmihaila commented Feb 14, 2016

Yep, but not in GetCapabilitiesExecutor, as the name suggests.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Feb 14, 2016

Member

Maybe it would be clearer if you thought of it as "ExecutorThatGetsCapabilities"

Member

domenic commented Feb 14, 2016

Maybe it would be clearer if you thought of it as "ExecutorThatGetsCapabilities"

@raulsebastianmihaila

This comment has been minimized.

Show comment
Hide comment
@raulsebastianmihaila

raulsebastianmihaila Feb 14, 2016

👍 Yep, it would be even clearer if it was named that way. :)

raulsebastianmihaila commented Feb 14, 2016

👍 Yep, it would be even clearer if it was named that way. :)

@bterlson

This comment has been minimized.

Show comment
Hide comment
@bterlson

bterlson Feb 17, 2016

Member

I think it's fine as-is naming wise but would accept a PR for a informative note regarding the purpose of this function.

Member

bterlson commented Feb 17, 2016

I think it's fine as-is naming wise but would accept a PR for a informative note regarding the purpose of this function.

@bterlson bterlson closed this Feb 17, 2016

@rauschma

This comment has been minimized.

Show comment
Hide comment
@rauschma

rauschma Apr 16, 2016

A class (or object) diagram would also be helpful, to get an overview of the entities that are involved in this part of the spec (PromiseReaction Record, PromiseCapability Record, Promise Reject Function, Promise Resolve Function, CapabilitiesExecutor Function, PromiseReactionJob, PromiseResolveThenableJob, Promise).

rauschma commented Apr 16, 2016

A class (or object) diagram would also be helpful, to get an overview of the entities that are involved in this part of the spec (PromiseReaction Record, PromiseCapability Record, Promise Reject Function, Promise Resolve Function, CapabilitiesExecutor Function, PromiseReactionJob, PromiseResolveThenableJob, Promise).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment