-
Notifications
You must be signed in to change notification settings - Fork 74
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
refactor(daemon): Parameterize provide type assertion #2210
Conversation
bc2428a
to
b5523a9
Compare
5285f30
to
723b688
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.
This is a great change. Below, I provide a suggestion and one finding concerning Provide
.
packages/daemon/src/types.d.ts
Outdated
export type Provide = <T extends string>( | ||
id: string, | ||
type?: T, | ||
) => Promise< | ||
T extends 'endo' | ||
? EndoBootstrap | ||
: T extends 'pet-store' | ||
? PetStore | ||
: T extends 'peer' | ||
? EndoGateway | ||
: T extends 'directory' | ||
? EndoDirectory | ||
: T extends 'network' | ||
? EndoNetwork | ||
: T extends 'host' | ||
? EndoHost | ||
: T extends 'hub' | ||
? NameHub | ||
: T extends 'readable-blob' | ||
? EndoReadable | ||
: T extends 'worker' | ||
? EndoWorker | ||
: T extends 'agent' | ||
? EndoAgent | ||
: T extends 'guest' | ||
? EndoGuest | ||
: T extends 'invitation' | ||
? Invitation | ||
: unknown | ||
>; |
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.
A couple of things regarding this one. First, I propose we reimplement it as follows:
- Establish a private type that only exists to map
Formula['type']
to the type of the value instantiated by a formula:
type FormulaTypeForValue = {
endo: EndoBootstrap,
host: EndoHost,
guest: EndoGuest,
worker: EndoWorker,
// ...
};
- Replace the ternary chain above with:
export type Provide = <T extends Formula['type'], U extends FormulaTypeForValue[T]>(
id: string,
type?: T,
) => Promise<U>;
This has the benefit of being more succinct and partially self-validating since the definition of Provide
errors if FormulaTypeForValue
does not have a key for every value in the string literal union Formula['type']
. (It's still possible for keyof FormulaTypeForValue
to superset Formula['type']
, but that will cause type errors at the point of use.)
Second, as it happens, Formula['type']
in fact does not include all of the string literals used as the second argument to provide()
, specifically the following:
agent
hub
network
I had thought that there was a bijection between "formula types" and "kinds of values that can be provided". I think we should attempt to eliminate this discrepancy if possible. If we don't want to do that now, we can still implement my above suggestion with the following band-aid:
type Providables = Formula['type'] & 'agent' | 'hub' | 'network';
// FormulaTypeForValue modified as necessary
export type Provide = <T extends Providables, U extends FormulaTypeForValue[T]>( /* ... */
packages/daemon/src/daemon.js
Outdated
@@ -252,6 +252,14 @@ const makeDaemonCore = async ( | |||
/** @type {import('./types.js').WeakMultimap<Record<string | symbol, unknown>, string>['get']} */ | |||
const getIdForRef = ref => idForRef.get(ref); | |||
|
|||
/** @type {import('./types.js').Provide} */ | |||
const provide = id => |
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.
Just for explicitness:
const provide = id => | |
const provide = (id, _formulaType) => |
723b688
to
c7eadd7
Compare
c7eadd7
to
b622d46
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.
LGTM!
This change adds a vestigial second argument to
provide
that just hints to TypeScript the expected return type, resulting is much less verbose type narrowing. I’ve also advancedprovide
higher in the flow so it can eat all the forward-reference lint comments.