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

Generic Service/Resource types #502

Merged
merged 4 commits into from
Nov 14, 2024

Conversation

bennostein
Copy link
Contributor

@bennostein bennostein commented Nov 13, 2024

This PR adds type parameters to the SkipService and Resource types, allowing to give more precise types and stricter checking that keys/types match between initialData, SkipService#createGraph, and Resource#instantiate.

E.G. where before we had

initialData?: Record<string, Entry<Json, Json>[]>
resources?: Record<string, new (params: Record<string, string>) => Resource>;
createGraph(
  inputCollections: Record<string, EagerCollection<Json, Json>>,
  context: Context,
): Record<string, EagerCollection<Json, Json>>;

and care was required to match up the keys of the various records, as well as the more-precise-than-Json types of keys/values, now you can write something like

type Inputs = { input1 : EagerCollection<K1, V1>, input2: EagerCollection<K2, V2> }
type Outputs = { output1 : EagerCollection<K3, V3>, output2: EagerCollection<K4, V4> }

and define your service as SkipService<Inputs, Outputs> to get static checking that the various records agree and that the keys/values have the right type.

@bennostein
Copy link
Contributor Author

bennostein commented Nov 13, 2024

The one bit I wanted to do but couldn't get to work was to give more precise type bounds than Json on the initialData's keys and values.

Given e.g.

type Inputs = { input1 : EagerCollection<K1, V1>, input2: EagerCollection<K2, V2> }

what I have here (initialData?: { [Prop in keyof Inputs]: Entry<Json, Json>[] };) ensures that initialData has the keys "input1" and "input2" but not that the values conform to K1/V1 and K2/V2.

As far as I can tell in typescript docs, there's no way to access the type arguments of a type so as to map each Inputs[Prop] (which has the form EagerCollection<K, V>) to the corresponding Entry<K, V> type.

But maybe a typescript wizard knows how to do that? @mbouaziz @skiplabsdaniel

Copy link
Contributor

@jberdine jberdine left a comment

Choose a reason for hiding this comment

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

I like this, I think the more informative types improve the understandability of the api meaningfully.

Did you mean to not adapt the database example in a similar way to you did for the sum and sheet ones?

skipruntime-ts/api/src/api.ts Outdated Show resolved Hide resolved
skipruntime-ts/api/src/api.ts Outdated Show resolved Hide resolved
@bennostein
Copy link
Contributor Author

I like this, I think the more informative types improve the understandability of the api meaningfully.

Did you mean to not adapt the database example in a similar way to you did for the sum and sheet ones?

I skipped it because adapting it doesn't add much in the way of clarity or brevity, and the type inference does fine without annotations. But just added equivalent annotations/type aliases there for consistency

Copy link
Contributor

@jberdine jberdine left a comment

Choose a reason for hiding this comment

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

This LGTM. I have an idea to try for the dissatisfaction you noted with the type of initialData, but no need to block this on that.

@mbouaziz
Copy link
Contributor

mbouaziz commented Nov 14, 2024

I tried with

type InitialData<Inputs extends NamedCollections> = {
  [Name in keyof Inputs]: Inputs[Name] extends EagerCollection<infer K, infer V>
    ? Entry<K, V>[]
    : Entry<Json, Json>[];
};
.
.
.
  initialData?: InitialData<Inputs>;

and it found type errors in the HN example, probably legit

@jberdine
Copy link
Contributor

Yes, that is the approach I am doing right now, and yeah, those type errors look legit.

@jberdine
Copy link
Contributor

I pushed the approach I tried as #503 . It is not exactly the same as Mehdi proposed, in that I tried to avoid conditional types and infer, since I haven't had great experience with them being predictable and reliable. It is more invasive though.

@bennostein bennostein merged commit 5f6b0ae into SkipLabs:main Nov 14, 2024
5 checks passed
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.

3 participants