Skip to content

Conversation

@domenic
Copy link
Member

@domenic domenic commented Sep 7, 2018

Fixes #45.

/cc @ricea @annevk.

I don't know if I'll have time to update all the dependencies that would be broken by this change. I think the most prominent user is https://w3c.github.io/ServiceWorker/.

Copy link

@ricea ricea left a comment

Choose a reason for hiding this comment

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

lgtm from quick review. I haven't looked at the generated output yet.

1. <a>Resolve</a> |promise| with |stepsResult|.
1. Let |failureStepsWrapper| be the following steps, given |reason|:
1. Let |stepsResult| be the result of performing |failureSteps| given |reason|. If these steps threw an exception, <a>reject</a> |promise| with that exception.
1. <a>Resolve</a> |promise| with |stepsResult|.
Copy link

Choose a reason for hiding this comment

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

Reject?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah. If failureSteps return something, we resolve with that thing. If they throw we reject on the previous line.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Unfortunately reusing Promise.all seems rather hard for this, so I guess this is the way to go.

To <dfn id="waiting-for-all" export lt="wait for all|waiting for all">wait for all</dfn> of a <a>list</a> of promises |promises|, with success steps |successSteps| that take a <a>list</a> of JavaScript values and failure steps |failureSteps| that take a rejection reason JavaScript value, perform the following steps:

1. Let |rejectionHandler| be a built-in function that takes an argument |arg| and performs the following steps:
1. Perform |failureSteps| given |arg|.
Copy link
Member

Choose a reason for hiding this comment

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

Identation?

Copy link
Member Author

Choose a reason for hiding this comment

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

What's wrong with the indentation?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I misread.

To <dfn id="waiting-for-all" export lt="wait for all|waiting for all">wait for all</dfn> of a <a>list</a> of promises |promises|, with success steps |successSteps| that take a <a>list</a> of JavaScript values and failure steps |failureSteps| that take a rejection reason JavaScript value, perform the following steps:

1. Let |rejectionHandler| be a built-in function that takes an argument |arg| and performs the following steps:
1. Perform |failureSteps| given |arg|.
Copy link

Choose a reason for hiding this comment

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

If multiple promises reject, then failureSteps will be performed multiple times. Is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, good catch. This was a no-op in Promise.all, because the failure steps there are just to reject a promise, and rejecting multiple times does nothing. Here it is bad. I will fix.

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.

3 participants