- Notifications
You must be signed in to change notification settings - Fork 13.2k
Add Awaited<T> to core library#21613
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
rbuckton commented Feb 4, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
rbuckton commented Feb 4, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
This only solves the "Incorrect return types or errors with complex Promise chains" issue in #17077. This does not solve the "Incorrect eager resolution of the "awaited type" for a type variable" issue. |
ajafff commented Feb 4, 2018
There's a difference to |
src/lib/es5.d.ts Outdated
| /** Gets the type resulting from awaiting `T`. This does **not** recursively unwrap nested promises. */ | ||
| declaretypeAwaited<T>= | ||
| TextendsAwaitable<Awaitable<infer U>> ? U : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we figure that two levels deep is enough for most uses, and if anyone complains, we can always add more later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if we currently don't do better than two levels, this will do for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are using the Promise definition from our libs then you won't need anything deeper. Once this is shipping we can modify the definitions in DT to use it as well. Per my examples above: type T3 = Awaited<Promise<Promise<number>>> // number for any level of nesting using either Promise or PromiseLike.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually only need:
declaretypeAwaited<T>=TextendsAwaitable<infer U> ? U : Textends{then(...args: any[]): any;} ? never : T;I seemed to have forgotten to stage an additional change to es5.d.ts.
src/lib/es5.d.ts Outdated
| /** An object type that can be definitely be awaited. Do not inherit from this type. */ | ||
| declaretypeAwaitable<T>={then(onfulfilled: (value: T)=>any): any;}; | ||
| /** An object type that may not be awaitable. Do not inherit from this type. */ |
weswighamFeb 4, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this type isn't supposed to be extended/used, why not just inline the object type inside Awaited, below, since it's only used one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've considered that, but this is more readable when getting Quick Info on Awaited<T>. I don't even need Awaitable<T> if I inline it in the 2-3 places its used but that again reduces readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed NonAwaitable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to also remove Awaitable<T> and inline the definition into Awaited<T>. The two other places where it was referenced were fine as PromiseLike<T>.
weswigham left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It definitely seems better than the union with PromiseLike most positions took before from before, and we can definitely improve it in the future if we can.
We should audit how this affects definitelytyped, though.
Igorbek commented Feb 4, 2018
src/lib/es5.d.ts Outdated
| declaretypeParameterDecorator=(target: Object,propertyKey: string|symbol,parameterIndex: number)=>void; | ||
| declaretypePromiseConstructorLike=new<T>(executor: (resolve: (value?: T|PromiseLike<T>)=>void,reject: (reason?: any)=>void)=>void)=>PromiseLike<T>; | ||
| /** An object type that can be definitely be awaited. Do not inherit from this type. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"be definitely be"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/lib/es5.d.ts Outdated
| /** Gets the type resulting from awaiting `T`. This does **not** recursively unwrap nested promises. */ | ||
| declaretypeAwaited<T>= | ||
| TextendsAwaitable<Awaitable<infer U>> ? U : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if we currently don't do better than two levels, this will do for now.
rbuckton commented Feb 4, 2018
@ajafff I'll see if there's anything worth changing here. Despite the naming |
rbuckton commented Feb 5, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Sorry, this was incorrect. If the asyncfunctionfoo(){lety1: {then(): any;};letx1=awaity1;// Type of 'await' operand must either be a valid promise or must not contain a callable 'then' member.lety2: {then(onfulfilled: ()=>any): any;};letx2=awaity2;// never}The best approximation of an error we can provide here is to resolve the type to |
sandersn commented Feb 5, 2018
Can you run this on DefinitelyTyped before checking in? Any change to inference is likely to disturb things there and it would be nice to know how big the break is. |
mhegazy commented Feb 5, 2018
Please also run the RWC tests before submitting and ensure we have not introduced any regressions there either. |
mhegazy commented Feb 5, 2018
and user tests. |
rbuckton commented Feb 5, 2018
@mhegazy, I plan to run this against RWC and DefinitelyTyped today. If all goes well, should we discuss this in the design meeting before taking the change? |
mhegazy commented Feb 5, 2018
sure. We should discuss the |
rbuckton commented Feb 6, 2018
@mhegazy There are a few failing RWC tests after this that may actually be more correct. Most of them seem to be related to assignability, i.e.: |
rbuckton commented Feb 6, 2018
An example of one of the RWC errors is this: I'd expect @ahejlsberg, any suggestions? Is there something I can change in my definition to address this issue, or is there a change we can make in the checker that makes sense? Is this something that |
KiaraGrouwstra commented Feb 11, 2018
I got it to recursively unwrap under the present definition of typeAwaited<T>={'0': T;'1': Awaited<Textends{then(onfulfilled: (value: infer U)=>any): any;} ? U : 'wat'>;}[Textends{then(onfulfilled: (value: any)=>any): any;} ? '1' : '0'];letone: 1;one=null!asAwaited<1>>;// okone=null!asAwaited<Promise<1>>>;// okone=null!asAwaited<Promise<Promise<1>>>>;// okI'll admit it's clunky and silly -- I just used an object for the conditional to enable type recursion. But it works on master already, fwiw. |
weswigham commented Feb 11, 2018
@tycho01 @ahejlsberg on typeAwaited<T>={'0': T;'1': Awaited<Textends{then(onfulfilled: (value: infer U)=>any): any;} ? U : never>;}[Textends{then(onfulfilled: (value: any)=>any): any;} ? '1' : '0'];functionfoo<T>(x: T){letone: T;letn1: Awaited<T>;letn2: Awaited<Awaited<T>>;letn3: Awaited<Awaited<Awaited<T>>>;one=n1;one=n2;one=n3;}spin forever. Looks like an infinite loop in |
rbuckton commented Feb 12, 2018
@weswigham, I think that is the same issue as in #21611 |
KiaraGrouwstra commented Feb 12, 2018
Sorry, just remembered I'd yet to test for the original core issue, |
KiaraGrouwstra commented Feb 14, 2018
I played around a bit more to get the union cases to work properly. Conditional types already distributed over union types properly, so this came down to finding a way to make recursive types based on this, i.e. somehow wrapping the recursion so it'd allow it, using some useless no-op wrapper delaying execution to trick it. I found a silly approach that appears to work: Using that to unwrap promises or to flatten nested array types: // unwrap a Promise to obtain its return valueexporttypeAwaited<T>={'1': Textends{then(onfulfilled: (value: infer U)=>any): any;} ? Awaited<U> : T;}[Textendsnumber ? '1' : '1'];// Awaited<Promise<1 | Promise<2>>> // -> 1 | 2// flatten a structure of nested tuples/arrays into a flat array typeexporttypeFlatten<T>={'1': Flatten<TextendsArray<infer U> ? U : T>;}[Textendsnumber ? '1' : '1'];// Flatten<Array<1 | Array<2>>> // -> 1 | 2 |
ericanderson commented Feb 15, 2018
@tycho01 you rock my friend |
KiaraGrouwstra commented Feb 17, 2018
Sorry, so far I'd tested through the compiler API, but through |
mhegazy commented Apr 24, 2018
Does not look like this is something we can proceed on at the time being.closing the PR, but leaving the branch for further investigations. |
This adds
Awaited<T>as a mechanism to unwrap a type to its "awaited" type:Unlike the
awaitexpression,Awaited<T>does not recursively unwrapTas we do not support circular references in type aliases. It turns out recursive unwrap ofTis mostly unnecessary if we modify the definition ofPromiseandPromiseLiketo return aPromise<Awaited<TResult>>:This effectively flattens
Promise<Promise<number>>as well as other nested definitions ofPromise:The
Awaited<T>type is a conditional type that provides the following conditions:Thas athen()method with a callback, infer the callback's first argument asUand use that as the result type.Thas athen()method without a callback, this is a non-promise "thenable" that can never be resolved, so the result type isnever.T.The benefit of this approach is more effective type inference from the
onfulfilledcallback passed to thethenmethod of aPromise: