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

feat(lib/es2020): Add Promise.allSettled(…) #34065

Merged

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Oct 12, 2019

Depends on #33707 (I’ve now inlined the Awaited<T> type)


Fixes #31083
Closes #31085


review?(@Kingwl)

Copy link
Contributor

@Kingwl Kingwl left a comment

Choose a reason for hiding this comment

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

LGTM

@trmaphi
Copy link

trmaphi commented Oct 24, 2019

LGTM

@graup
Copy link

graup commented Nov 7, 2019

I guess this code can be simplified similarly to #33707 ?

@SimonSchick
Copy link

Nope, typings for node still need to support 2.1 until further notice.

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Nov 7, 2019

I first need #33707 to be merged, so that I can get access to the Awaited<T> type.

@ExE-Boss ExE-Boss mentioned this pull request Nov 13, 2019
2 tasks
* @param values An array of Promises.
* @returns A new Promise.
*/
allSettled<T extends readonly any[] | readonly [any]>(values: T): Promise<{ -readonly [P in keyof T]: PromiseSettledResult<T[P] extends PromiseLike<infer U> ? U : T[P]> }>;
Copy link
Member

Choose a reason for hiding this comment

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

Should we make these readonly unknown[] | readonly [unknown]? (@RyanCavanaugh @rbuckton)

@@ -0,0 +1,29 @@
interface PromiseResolvedResult<T> {

Choose a reason for hiding this comment

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

Hi. Drive-by code review here. Shouldn’t this be named something like PromiseFulfilledResult to adhere to the states and fates terminology? I think the reason calling it PromiseResolvedResult is incorrect here is that while all fulfilled promises are resolved, not all resolved promises are fulfilled. A promise can be resolved to a pending promise, in which case it has not settled, and it can also be resolved to a rejected promise, in which case it has not fulfilled.

If my interpretation of states and fates terminology is correct, shouldn’t the correct name of this interface be PromiseFulfilledResult?

@ExE-Boss ExE-Boss force-pushed the lib/es2020/add-promise-allsettled branch from ec3628c to 2929aad Compare January 2, 2020 17:01
@DanielRosenwasser DanielRosenwasser merged commit 5a34274 into microsoft:master Jan 3, 2020
@DanielRosenwasser
Copy link
Member

Thank you!

@ExE-Boss ExE-Boss deleted the lib/es2020/add-promise-allsettled branch January 3, 2020 21:03
@SimonSchick
Copy link

Still feel like it should've used unknown for better type safety 🤷‍♂

@sanglt1902
Copy link

LGTM

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.

proposal-promise-allSettled