Skip to content
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

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

kriskowal
Copy link
Member

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 advanced provide higher in the flow so it can eat all the forward-reference lint comments.

@kriskowal kriskowal requested a review from rekmarks April 12, 2024 05:59
@kriskowal kriskowal force-pushed the kriskowal-daemon-maker-table branch from bc2428a to b5523a9 Compare April 13, 2024 08:25
@kriskowal kriskowal force-pushed the kriskowal-provide-type-parameter branch from 5285f30 to 723b688 Compare April 13, 2024 08:25
Copy link
Contributor

@rekmarks rekmarks left a 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.

Comment on lines 720 to 749
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
>;
Copy link
Contributor

@rekmarks rekmarks Apr 14, 2024

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:

  1. 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,
  // ...
};
  1. 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]>( /* ... */

@@ -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 =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for explicitness:

Suggested change
const provide = id =>
const provide = (id, _formulaType) =>

Base automatically changed from kriskowal-daemon-maker-table to master April 15, 2024 17:47
@kriskowal kriskowal force-pushed the kriskowal-provide-type-parameter branch from 723b688 to c7eadd7 Compare April 15, 2024 18:57
@kriskowal kriskowal force-pushed the kriskowal-provide-type-parameter branch from c7eadd7 to b622d46 Compare April 15, 2024 18:58
@kriskowal kriskowal enabled auto-merge April 15, 2024 18:58
Copy link
Contributor

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kriskowal kriskowal merged commit 13c52f1 into master Apr 15, 2024
18 checks passed
@kriskowal kriskowal deleted the kriskowal-provide-type-parameter branch April 15, 2024 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants