-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Fix Promise interfaces #22772
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
Fix Promise interfaces #22772
Conversation
src/lib/es5.d.ts
Outdated
@@ -1273,7 +1273,9 @@ declare type PropertyDecorator = (target: Object, propertyKey: string | symbol) | |||
declare type MethodDecorator = <T>(target: Object, propertyKey: string | symbol, descriptor: TypedPropertyDescriptor<T>) => TypedPropertyDescriptor<T> | void; | |||
declare type ParameterDecorator = (target: Object, propertyKey: string | symbol, parameterIndex: number) => void; | |||
|
|||
declare type PromiseConstructorLike = new <T>(executor: (resolve: (value?: T | PromiseLike<T>) => void, reject: (reason?: any) => void) => void) => PromiseLike<T>; | |||
interface PromiseConstructorLike { |
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 don't think this specific change is necessary. PromiseConstructorLike
is only used by the type system to determine if an explicit Promise
-like return type in a down-level ES5 async function can be used as a Promise
constructor. Changing the type could adversely affect people using this behavior.
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.
Indeed, this change may make compatibility worse. I'll revert this.
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.
Done.
@@ -10,7 +10,7 @@ interface PromiseConstructor { | |||
* a resolve callback used resolve the promise with a value or the result of another promise, | |||
* and a reject callback used to reject the promise with a provided reason or error. | |||
*/ | |||
new <T>(executor: (resolve: (value?: T | PromiseLike<T>) => void, reject: (reason?: any) => void) => void): Promise<T>; | |||
new <T>(executor: (resolve: [T] extends [void] ? (value?: T | PromiseLike<T>) => void : (value: T | PromiseLike<T>) => void, reject: (reason?: any) => void) => void): Promise<T>; |
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.
Is the goal to only allow value
to be optional if T
is exactly void
? If T
is void | number
you end up with the second definition (with a non-optional value
) for (value: void | number | PromiseLike<void | number>) => void
.
You could try this instead:
resolve: [void] extends [T] ? (value?: T | PromiseLike<T>) => void : (value: T | PromiseLike<T>) => void
By inverting the condition, you would get cases where void
is assignable (such as when T
is a union including void
).
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 seems a nice idea. I'll do it.
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.
Done.
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 realized [void] extends [T]
doesn't work with undefined
. So I reverted it.
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.
That's unfortunate, but its better than nothing I suppose.
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.
Have you tried this without the conditional type? This should work as well and is probably easier to read:
new <T extends void>(executor: (resolve: (value?: T | PromiseLike<T>) => void, reject: (reason?: any) => void) => void): Promise<T>;
new <T>(executor: (resolve: (value: T | PromiseLike<T>) => void, reject: (reason?: any) => void) => void): Promise<T>;
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.
As I mentioned at #21867 (comment), it is not extensible.
@rbuckton Ah, I realized a problem now. In the following case, this change make an unfavorable effect. class C<T> extends Promise<T> {
constructor() {
super(resolve => resolve(0 as any)); // error, can't call it.
}
} |
@rbuckton Our RWC tests show this as kinda breaky, for a few reasons.
|
I'm reverting this change for now. |
Revert change to PromiseConstructor in #22772
Fixes #11094, #3356, #21867.
@mhegazy @rbuckton Can you include this to the next release?