Add type signature for Promise.allSettled() #8230
Conversation
jamesisaac
added
the
Library definitions
There was a problem hiding this comment.
Overall this looks good, and it appears to match the spec. However, I'd like to see some tests added just to make sure Flow behaves the way we expect for these types.
I think they belong in tests/core_tests. Update the test output for this test but don't worry about re-recording the rest -- I can do that when I import.
lib/core.js
Outdated
| static race<T, Elem: Promise<T> | T>(promises: Iterable<Elem>): Promise<T>; | ||
| } | ||
|
|
||
| // we use this signature when typing await expressions | ||
| declare function $await<T>(p: Promise<T> | T): T; | ||
|
|
||
| declare function $settlement<T>(p: Promise<T> | T): SettlementResult<T>; | ||
| declare type SettlementResult<T> = { status: 'fulfilled', value: T } | { status: 'rejected', reason: any }; |
There was a problem hiding this comment.
I have a few requests for this line:
- These types ought to be exact -- use
{|and|}instead of{and}(this will be the default soon anyway). - This will be more usable if the type parameter is covariant, so write
SettlementResult<+T>instead ofSettlementResult<T>.- This will also force you to make at least the
valueproperty covariant by adding a+in front of it. And you might as well just make the other fields covariant too, while you're at it.
- This will also force you to make at least the
- I'd rather see this use
mixedfor the rejection reason instead ofany
There was a problem hiding this comment.
Thank you for the substantive feedback! I've made a changes on all the items.
|
cc @moroine |
|
@nmote Thanks for the hint about tests. I am not sure I get the tests idea right, but hope I wrote them in the right way. :) |
|
Thanks for updating this! Just today, somebody else who wanted to use |
|
Leaving this open in case you'd like to add your tests |
|
Well, I am always glad to help by all means. :) Am I getting you right, you are suggesting me to resolve the conflict between the two implementations (in favour of the already merged one) and add my tests to the existing ones? It seems reasonable. Which way is preferred -- merging the |
|
Closing this since it hasn't been updated for a long time. |
I miss the type signature for the new (ES2020) static method of the built-in Promise so I've decided to add it.
Documentation:
The change closes one of many items in #560