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.

Sign up for GitHub

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

Jump to bottom

Better typings for Promise, like #31117 #33707

Closed
wants to merge 4 commits into from
Closed

Better typings for Promise, like #31117 #33707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8381658
Move this Promise.race() overload back to es2015.iterable.d.ts
jablko Aug 23, 2019
d839f22
Add tests
jablko Nov 20, 2019
4544384
Better typings for Promise.all(), like #31117
jablko Aug 27, 2019
ab5117a
Add tests
jablko Aug 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/lib/es2015.iterable.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,18 +200,18 @@ interface PromiseConstructor {
/**
* Creates a Promise that is resolved with an array of results when all of the provided Promises
* resolve, or rejected when any Promise is rejected.
* @param values An array of Promises.
* @param values An iterable of Promises.
* @returns A new Promise.
*/
all<TAll>(values: Iterable<TAll | PromiseLike<TAll>>): Promise<TAll[]>;
all<TAll>(values: Iterable<TAll>): Promise<(TAll extends undefined ? TAll : TAll extends PromiseLike<infer UAll> ? UAll : TAll)[]>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
all<TAll>(values: Iterable<TAll>): Promise<(TAll extends undefined ? TAll : TAll extends PromiseLike<infer UAll> ? UAll : TAll)[]>;
all<TAll>(values: Iterable<TAll>): Promise<(TAll extends PromiseLike<infer UAll> ? UAll : TAll)[]>;


/**
* Creates a Promise that is resolved or rejected when any of the provided Promises are resolved
* or rejected.
* @param values An array of Promises.
* @param values An iterable of Promises.
* @returns A new Promise.
*/
race<T>(values: Iterable<T | PromiseLike<T>>): Promise<T>;
Copy link
Member

Choose a reason for hiding this comment

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

You removed race here, was that intended?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why was race removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

4841984 it was moved to es2015.promise.d.ts in #31117

Copy link
Member

Choose a reason for hiding this comment

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

It existed here so that you could include es2015.promise without es2015.iterable, depending on what shims/polyfills are used down-level, although it looks like a definition including Iterable was incorrectly added to that file at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to fix that but just wondering, is it better to be able to include es2015.promise without es2015.iterable, or es2015.iterable without es2015.promise? The latter is more obvious (all Promise.all() and Promise.race() definitions located together) and has fewer overloads (since readonly T[] extends Iterable<T>).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I get it now: es2015.iterable doesn't depend on PromiseConstructor, it augments it. I'll move that Iterable overload back to es2015.iterable.

Copy link
Contributor Author

@jablko jablko Nov 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

✔️ Done.

race<T>(values: Iterable<T>): Promise<T extends undefined ? T : T extends PromiseLike<infer U> ? U : T>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
race<T>(values: Iterable<T>): Promise<T extends undefined ? T : T extends PromiseLike<infer U> ? U : T>;
race<T>(values: Iterable<T>): Promise<T extends PromiseLike<infer U> ? U : T>;

}

declare namespace Reflect {
Expand Down
91 changes: 8 additions & 83 deletions src/lib/es2015.promise.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
// The undefined case is for strictNullChecks false, in which case
// undefined extends PromiseLike<infer U> is true, which would otherwise
// make Awaited<undefined> -> unknown.
type Awaited<T> = T extends undefined ? T : T extends PromiseLike<infer U> ? U : T;
Copy link
Member

Choose a reason for hiding this comment

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

The primary reason we have not introduced this type to this point is that it does not accurately represent the recursive nature of await, so an Awaited<Promise<Promise<number>> ends up being Promise<number>, while an await Promise.resolve(Promise.resolve(1)) ends up being 1.

This also has a problem in that it incorrectly handles "thenables" (i.e. objects with a callable "then" member that are not promises):

type X = Awaited<{ then(): void }>; // X is `{ then(): void }`

In reality, awaiting an object like this will produce a Promise that never resolves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A problem with Promise<Promise<number>> is that it's equivalent to Promise<number>, but not assignable to it. Instead of making Awaited recursive, what do you think of this attempt to resolve #27711, and avoid nested promises in the first place?

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 still possible to write Promise<Promise<number>> and get the wrong experience, which is unfortunate. We've been investigating a number of ways to resolve this for some time now. Until we have a firm decision on how to handle nested promises, I'm not sure I could approve this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of a solution like the one in #31117 in the mean time?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type Awaited<T> = T extends undefined ? T : T extends PromiseLike<infer U> ? U : T;
type Awaited<T> = T extends PromiseLike<infer U> ? U : T;


interface PromiseConstructor {
/**
* A reference to the prototype.
Expand All @@ -18,95 +23,15 @@ interface PromiseConstructor {
* @param values An array of Promises.
* @returns A new Promise.
*/
all<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10>(values: readonly [T1 | PromiseLike<T1>, T2 | PromiseLike<T2>, T3 | PromiseLike<T3>, T4 | PromiseLike <T4>, T5 | PromiseLike<T5>, T6 | PromiseLike<T6>, T7 | PromiseLike<T7>, T8 | PromiseLike<T8>, T9 | PromiseLike<T9>, T10 | PromiseLike<T10>]): Promise<[T1, T2, T3, T4, T5, T6, T7, T8, T9, T10]>;

/**
* Creates a Promise that is resolved with an array of results when all of the provided Promises
* resolve, or rejected when any Promise is rejected.
* @param values An array of Promises.
* @returns A new Promise.
*/
all<T1, T2, T3, T4, T5, T6, T7, T8, T9>(values: readonly [T1 | PromiseLike<T1>, T2 | PromiseLike<T2>, T3 | PromiseLike<T3>, T4 | PromiseLike <T4>, T5 | PromiseLike<T5>, T6 | PromiseLike<T6>, T7 | PromiseLike<T7>, T8 | PromiseLike<T8>, T9 | PromiseLike<T9>]): Promise<[T1, T2, T3, T4, T5, T6, T7, T8, T9]>;

/**
* Creates a Promise that is resolved with an array of results when all of the provided Promises
* resolve, or rejected when any Promise is rejected.
* @param values An array of Promises.
* @returns A new Promise.
*/
all<T1, T2, T3, T4, T5, T6, T7, T8>(values: readonly [T1 | PromiseLike<T1>, T2 | PromiseLike<T2>, T3 | PromiseLike<T3>, T4 | PromiseLike <T4>, T5 | PromiseLike<T5>, T6 | PromiseLike<T6>, T7 | PromiseLike<T7>, T8 | PromiseLike<T8>]): Promise<[T1, T2, T3, T4, T5, T6, T7, T8]>;

/**
* Creates a Promise that is resolved with an array of results when all of the provided Promises
* resolve, or rejected when any Promise is rejected.
* @param values An array of Promises.
* @returns A new Promise.
*/
all<T1, T2, T3, T4, T5, T6, T7>(values: readonly [T1 | PromiseLike<T1>, T2 | PromiseLike<T2>, T3 | PromiseLike<T3>, T4 | PromiseLike <T4>, T5 | PromiseLike<T5>, T6 | PromiseLike<T6>, T7 | PromiseLike<T7>]): Promise<[T1, T2, T3, T4, T5, T6, T7]>;

/**
* Creates a Promise that is resolved with an array of results when all of the provided Promises
* resolve, or rejected when any Promise is rejected.
* @param values An array of Promises.
* @returns A new Promise.
*/
all<T1, T2, T3, T4, T5, T6>(values: readonly [T1 | PromiseLike<T1>, T2 | PromiseLike<T2>, T3 | PromiseLike<T3>, T4 | PromiseLike <T4>, T5 | PromiseLike<T5>, T6 | PromiseLike<T6>]): Promise<[T1, T2, T3, T4, T5, T6]>;

/**
* Creates a Promise that is resolved with an array of results when all of the provided Promises
* resolve, or rejected when any Promise is rejected.
* @param values An array of Promises.
* @returns A new Promise.
*/
all<T1, T2, T3, T4, T5>(values: readonly [T1 | PromiseLike<T1>, T2 | PromiseLike<T2>, T3 | PromiseLike<T3>, T4 | PromiseLike <T4>, T5 | PromiseLike<T5>]): Promise<[T1, T2, T3, T4, T5]>;

/**
* Creates a Promise that is resolved with an array of results when all of the provided Promises
* resolve, or rejected when any Promise is rejected.
* @param values An array of Promises.
* @returns A new Promise.
*/
all<T1, T2, T3, T4>(values: readonly [T1 | PromiseLike<T1>, T2 | PromiseLike<T2>, T3 | PromiseLike<T3>, T4 | PromiseLike <T4>]): Promise<[T1, T2, T3, T4]>;

/**
* Creates a Promise that is resolved with an array of results when all of the provided Promises
* resolve, or rejected when any Promise is rejected.
* @param values An array of Promises.
* @returns A new Promise.
*/
all<T1, T2, T3>(values: readonly [T1 | PromiseLike<T1>, T2 | PromiseLike<T2>, T3 | PromiseLike<T3>]): Promise<[T1, T2, T3]>;

/**
* Creates a Promise that is resolved with an array of results when all of the provided Promises
* resolve, or rejected when any Promise is rejected.
* @param values An array of Promises.
* @returns A new Promise.
*/
all<T1, T2>(values: readonly [T1 | PromiseLike<T1>, T2 | PromiseLike<T2>]): Promise<[T1, T2]>;

/**
* Creates a Promise that is resolved with an array of results when all of the provided Promises
* resolve, or rejected when any Promise is rejected.
* @param values An array of Promises.
* @returns A new Promise.
*/
all<T>(values: readonly (T | PromiseLike<T>)[]): Promise<T[]>;
all<T extends readonly any[]>(values: T): Promise<{ -readonly [P in keyof T]: Awaited<T[P]> }>;
Copy link

Choose a reason for hiding this comment

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

Given this overload, are the tuple-based overloads even necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right! I've now updated this PR accordingly. Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the trick described in DefinitelyTyped/DefinitelyTyped#41098, this should probably be:

Suggested change
all<T extends readonly any[]>(values: T): Promise<{ -readonly [P in keyof T]: Awaited<T[P]> }>;
all<T extends readonly any[] | readonly [any]>(values: T): Promise<{ -readonly [P in keyof T]: Awaited<T[P]> }>;


/**
* Creates a Promise that is resolved or rejected when any of the provided Promises are resolved
* or rejected.
* @param values An array of Promises.
* @returns A new Promise.
*/
race<T>(values: readonly T[]): Promise<T extends PromiseLike<infer U> ? U : T>;

/**
* Creates a Promise that is resolved or rejected when any of the provided Promises are resolved
* or rejected.
* @param values An iterable of Promises.
* @returns A new Promise.
*/
race<T>(values: Iterable<T>): Promise<T extends PromiseLike<infer U> ? U : T>;
race<T extends readonly any[]>(values: T): Promise<Awaited<T[number]>>;

/**
* Creates a new rejected promise for the provided reason.
Expand All @@ -123,7 +48,7 @@ interface PromiseConstructor {
resolve<T>(value: T | PromiseLike<T>): Promise<T>;

/**
* Creates a new resolved promise .
* Creates a new resolved promise.
* @returns A resolved promise.
*/
resolve(): Promise<void>;
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/asyncArrowFunction5_es2017.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ tests/cases/conformance/async/es2017/asyncArrowFunction/asyncArrowFunction5_es20
!!! error TS1005: ',' expected.
~~~~~~~
!!! error TS2403: Subsequent variable declarations must have the same type. Variable 'Promise' must be of type 'PromiseConstructor', but here has type 'any'.
!!! related TS6203 /.ts/lib.es2015.promise.d.ts:152:13: 'Promise' was also declared here.
!!! related TS6203 /.ts/lib.es2015.promise.d.ts:77:13: 'Promise' was also declared here.
~
!!! error TS1005: ',' expected.
~~
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/asyncArrowFunction5_es5.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ tests/cases/conformance/async/es5/asyncArrowFunction/asyncArrowFunction5_es5.ts(
!!! error TS1005: ',' expected.
~~~~~~~
!!! error TS2403: Subsequent variable declarations must have the same type. Variable 'Promise' must be of type 'PromiseConstructor', but here has type 'any'.
!!! related TS6203 /.ts/lib.es2015.promise.d.ts:152:13: 'Promise' was also declared here.
!!! related TS6203 /.ts/lib.es2015.promise.d.ts:77:13: 'Promise' was also declared here.
~
!!! error TS1005: ',' expected.
~~
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/asyncArrowFunction5_es6.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ tests/cases/conformance/async/es6/asyncArrowFunction/asyncArrowFunction5_es6.ts(
!!! error TS1005: ',' expected.
~~~~~~~
!!! error TS2403: Subsequent variable declarations must have the same type. Variable 'Promise' must be of type 'PromiseConstructor', but here has type 'any'.
!!! related TS6203 /.ts/lib.es2015.promise.d.ts:152:13: 'Promise' was also declared here.
!!! related TS6203 /.ts/lib.es2015.promise.d.ts:77:13: 'Promise' was also declared here.
~
!!! error TS1005: ',' expected.
~~
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/asyncArrowFunction9_es2017.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ tests/cases/conformance/async/es2017/asyncArrowFunction/asyncArrowFunction9_es20
!!! error TS1005: ',' expected.
~~~~~~~
!!! error TS2403: Subsequent variable declarations must have the same type. Variable 'Promise' must be of type 'PromiseConstructor', but here has type 'any'.
!!! related TS6203 /.ts/lib.es2015.promise.d.ts:152:13: 'Promise' was also declared here.
!!! related TS6203 /.ts/lib.es2015.promise.d.ts:77:13: 'Promise' was also declared here.
~
!!! error TS1005: ',' expected.
~~
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/asyncArrowFunction9_es5.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ tests/cases/conformance/async/es5/asyncArrowFunction/asyncArrowFunction9_es5.ts(
!!! error TS1005: ',' expected.
~~~~~~~
!!! error TS2403: Subsequent variable declarations must have the same type. Variable 'Promise' must be of type 'PromiseConstructor', but here has type 'any'.
!!! related TS6203 /.ts/lib.es2015.promise.d.ts:152:13: 'Promise' was also declared here.
!!! related TS6203 /.ts/lib.es2015.promise.d.ts:77:13: 'Promise' was also declared here.
~
!!! error TS1005: ',' expected.
~~
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/asyncArrowFunction9_es6.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ tests/cases/conformance/async/es6/asyncArrowFunction/asyncArrowFunction9_es6.ts(
!!! error TS1005: ',' expected.
~~~~~~~
!!! error TS2403: Subsequent variable declarations must have the same type. Variable 'Promise' must be of type 'PromiseConstructor', but here has type 'any'.
!!! related TS6203 /.ts/lib.es2015.promise.d.ts:152:13: 'Promise' was also declared here.
!!! related TS6203 /.ts/lib.es2015.promise.d.ts:77:13: 'Promise' was also declared here.
~
!!! error TS1005: ',' expected.
~~
Expand Down
8 changes: 7 additions & 1 deletion tests/baselines/reference/correctOrderOfPromiseMethod.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ async function countEverything(): Promise<number> {
const [resultA, resultB] = await Promise.all([
providerA(),
providerB(),
] as const);
]);

const dataA: A[] = resultA;
const dataB: B[] = resultB;
Expand All @@ -24,6 +24,10 @@ async function countEverything(): Promise<number> {
}
return 0;
}

// #31179

const expected: Promise<["a", "b", "c"]> = Promise.all(undefined as readonly ["a", "b", "c"]);


//// [correctOrderOfPromiseMethod.js]
Expand Down Expand Up @@ -92,3 +96,5 @@ function countEverything() {
});
});
}
// #31179
var expected = Promise.all(undefined);
16 changes: 13 additions & 3 deletions tests/baselines/reference/correctOrderOfPromiseMethod.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,17 @@ async function countEverything(): Promise<number> {
const [resultA, resultB] = await Promise.all([
>resultA : Symbol(resultA, Decl(correctOrderOfPromiseMethod.ts, 13, 11))
>resultB : Symbol(resultB, Decl(correctOrderOfPromiseMethod.ts, 13, 19))
>Promise.all : Symbol(PromiseConstructor.all, Decl(lib.es2015.iterable.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --) ... and 6 more)
>Promise.all : Symbol(PromiseConstructor.all, Decl(lib.es2015.iterable.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --))
>Promise : Symbol(Promise, Decl(lib.es5.d.ts, --, --), Decl(lib.es2015.iterable.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --), Decl(lib.es2015.symbol.wellknown.d.ts, --, --))
>all : Symbol(PromiseConstructor.all, Decl(lib.es2015.iterable.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --) ... and 6 more)
>all : Symbol(PromiseConstructor.all, Decl(lib.es2015.iterable.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --))

providerA(),
>providerA : Symbol(providerA, Decl(correctOrderOfPromiseMethod.ts, 10, 9))

providerB(),
>providerB : Symbol(providerB, Decl(correctOrderOfPromiseMethod.ts, 11, 9))

] as const);
]);

const dataA: A[] = resultA;
>dataA : Symbol(dataA, Decl(correctOrderOfPromiseMethod.ts, 18, 9))
Expand All @@ -70,3 +70,13 @@ async function countEverything(): Promise<number> {
return 0;
}

// #31179

const expected: Promise<["a", "b", "c"]> = Promise.all(undefined as readonly ["a", "b", "c"]);
>expected : Symbol(expected, Decl(correctOrderOfPromiseMethod.ts, 28, 5))
>Promise : Symbol(Promise, Decl(lib.es5.d.ts, --, --), Decl(lib.es2015.iterable.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --), Decl(lib.es2015.symbol.wellknown.d.ts, --, --))
>Promise.all : Symbol(PromiseConstructor.all, Decl(lib.es2015.iterable.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --))
>Promise : Symbol(Promise, Decl(lib.es5.d.ts, --, --), Decl(lib.es2015.iterable.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --), Decl(lib.es2015.symbol.wellknown.d.ts, --, --))
>all : Symbol(PromiseConstructor.all, Decl(lib.es2015.iterable.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --))
>undefined : Symbol(undefined)

Loading