-
Notifications
You must be signed in to change notification settings - Fork 228
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
improve types of fulfillment helpers #6976
Conversation
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.
Looks good but I'll hold approval until clarification on the external dependency, and the fix for extends callable vs Function
@@ -3,7 +3,7 @@ const { fromEntries, keys, values } = Object; | |||
/** @type { <X, Y>(xs: X[], ys: Y[]) => [X, Y][]} */ | |||
export const zip = (xs, ys) => harden(xs.map((x, i) => [x, ys[+i]])); | |||
|
|||
/** @type { <K extends string, V>(obj: Record<K, ERef<V>>) => Promise<Record<K, V>> } */ | |||
/** @type { <T extends Record<string, ERef<any>>>(obj: T) => Promise<{ [K in keyof T]: Awaited<T[K]>}> } */ |
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.
🙏
packages/internal/src/utils.js
Outdated
@@ -250,16 +250,12 @@ harden(bindAllMethods); | |||
|
|||
/** | |||
* @template {{}} T | |||
* @typedef {{ [K in keyof T]: DeeplyAwaited<T[K]> }} DeeplyAwaitedObject | |||
* @typedef {{ [K in keyof T]: T[K] extends Function ? T[K] : DeeplyAwaited<T[K]> }} DeeplyAwaitedObject |
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.
* @typedef {{ [K in keyof T]: T[K] extends Function ? T[K] : DeeplyAwaited<T[K]> }} DeeplyAwaitedObject | |
* @typedef {{ [K in keyof T]: T[K] extends (...args: any[]) => any ? T[K] : DeeplyAwaited<T[K]> }} DeeplyAwaitedObject |
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 with aCallable
type
packages/internal/src/utils.js
Outdated
* @template T | ||
* @typedef {T extends PromiseLike<any> ? Awaited<T> : T extends {} ? DeeplyAwaitedObject<T> : Awaited<T>} DeeplyAwaited | ||
* @typedef {T extends PromiseLike<any> ? Awaited<T> : T extends {} ? import('type-fest').Simplify<DeeplyAwaitedObject<T>> : Awaited<T>} DeeplyAwaited |
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 this the first time we take an external dependency for our types? Wondering how that works out once we start publishing our packages correctly and the exported types have external dependencies (there is an argument to be made in that case that it is not in fact a dev dependency)
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.
Good catch! yarn prepack
showed this change didn't even build.
It's such a small type I backed out the import and copied it over with attribution.
9d03e4c
to
2683f47
Compare
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.
Thanks for these improvements!
* @typedef {{[KeyType in keyof T]: T[KeyType]} & {}} Simplify | ||
* flatten the type output to improve type hints shown in editors |
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.
Neat
Description
Split from #6963 for easier review.
Security Considerations
--
Scaling Considerations
--
Documentation Considerations
--
Testing Considerations
New tests