Fix Promise interfaces#22772
Conversation
| 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.
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.
Indeed, this change may make compatibility worse. I'll revert this.
| * 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.
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>) => voidBy 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.
It seems a nice idea. I'll do it.
There was a problem hiding this comment.
I realized [void] extends [T] doesn't work with undefined. So I reverted it.
There was a problem hiding this comment.
That's unfortunate, but its better than nothing I suppose.
There was a problem hiding this comment.
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.
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?