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

co.json: Allow interface types from generics #470

Closed
gdorsi opened this issue Oct 1, 2024 · 11 comments · Fixed by #492
Closed

co.json: Allow interface types from generics #470

gdorsi opened this issue Oct 1, 2024 · 11 comments · Fixed by #492
Assignees

Comments

@gdorsi
Copy link
Contributor

gdorsi commented Oct 1, 2024

The co.json type factory accepts a generic to let the Jazz users define the expected shape.

We want to make it possible to pass as argument only types that can be serialized, which means that anything that includes functions shouldn't be accepted as valid type.

The current implementation doesn't accept valid interface types, leading to some confusion when trying to use co.json correctly.

We want to fix the generic type in order to work in the following way:

type ValidJSON = {
    prop: string;
    prop2: number;
}

// This is valid ✅
co.json<ValidJSON>();

type NotValidJSON = {
    date: Date;
}

// This isn't valid ✅
co.json<NotValidJSON>();

type NotValidJSON2 = {
    fn: () => void;
}

// This isn't valid ✅
co.json<NotValidJSON2>();

interface ValidInterface {
    prop: string;
    prop2: number;
}

// This is flagged as error but should be valid ❌
co.json<ValidInterface>();
@milkcask
Copy link

milkcask commented Oct 2, 2024

I came across this discussion in Svelte some time ago: sveltejs/kit#1997. I can share some background info.

There are many behaviour differences between interfaces and type literals in Typescript and unfortunately this is one of them. This issue is rooted in the design choice of Typescript where interfaces do not carry an implicit index signature while type literals do. See microsoft/TypeScript#15300. ^

In other words, co.json accepts ValidJSON because ValidJSON’s implicit index signature is the same as the index signature in JsonObject. There are workarounds that involve creating utilities to sort of convert interfaces into types. But there is still no straightforward way to apply these utilities automagically.

From a DX perspective, is providing a utility along with clear instructions and examples for devs sufficient?

Note:
^ Internally, it used to be done by a check in getImplicitIndexTypeOfType (release-2.0/src/compiler/checker.ts#L4532) when implicit index signatures were first introduced in 2016, but it has become a part of type inference since around version 3.7, see isObjectTypeWithInferableIndex (v5.6.2/src/compiler/checker.ts#L25083).

@gdorsi
Copy link
Contributor Author

gdorsi commented Oct 3, 2024

Thanks @milkcask!

From a DX perspective, is providing a utility along with clear instructions and examples for devs sufficient?

We can give it a try, do you have any examples that we could follow?

For now we are suggesting this as workaround which actually turns off our checks:

export class MyCoMap extends CoMap {
    data = co.json<unknown>() as co<MyInterface>
}

Anything better than this is highly appreciated.

@gdorsi
Copy link
Contributor Author

gdorsi commented Oct 3, 2024

Found the answer in the linked issue, but our use case is a little more tricky because it is a property definition.

type Typify<T> = { [K in keyof T]: Typify<T[K]> };

interface ValidInterface {
    prop: string;
    prop2: number;
}

export class MyCoMap extends CoMap { 
    // This works, yay!
    data = co.json<Typify<ValidInterface>>();
}

const data: ValidInterface = {
    prop: "test",
    prop2: 123,
};

// This doesn't validate and would need casting
MyCoMap.create({ data }, { owner: me });

@milkcask
Copy link

milkcask commented Oct 3, 2024

@gdorsi Your snippet lgtm and should work. What is tsc complaining about on your end?

Screenshot 2024-10-03 at 14 33 42

I usually use this to avoid unnecessary recursion:

type InferIndexSignature<T> = { [K in keyof T & keyof T]: T[K] };

@gdorsi
Copy link
Contributor Author

gdorsi commented Oct 3, 2024

@milkcask Confirm that it works!

Not sure why it didn't this morning 🤔
Probably it was the VSCode cache playing pranks to me 🤷

Thanks for the suggestion and for double-checking it!

@gdorsi
Copy link
Contributor Author

gdorsi commented Oct 3, 2024

@aeplay

I think that we can proceed as @milkcask suggested and provide an utility type + some documentation to let our users handle this use case.

WDYT?

@aeplay
Copy link
Contributor

aeplay commented Oct 3, 2024

I wonder why we can't do a conditional Generic gate like Typify<T> extends JsonObject ? Typify<T> : never

Am I missing something?

@gdorsi
Copy link
Contributor Author

gdorsi commented Oct 4, 2024

@aeplay

Do you mean something like this?

Screenshot 2024-10-04 at 18 06 03

I had the same idea, but it means that the error wouldn't be flagged when passing the type into the generic but when calling the create function making the type error harder to debug.

@milkcask
Copy link

milkcask commented Oct 4, 2024

I think the homomorphic version of Typify / InferIndexSignature can be adjusted so that it extends (or intersects) JsonValue.

type TypifiedJsonValue<T> = { [K in keyof T & string]: T[K] extends JsonValue | undefined ? T[K] : never };

json<T extends JsonValue | TypifiedJsonValue<T>>(): co<T> { ... }

It’s interesting that this works.

@aeplay Yes, that should work too, but a drawback is that when an error occurs, the error elaboration stops at the bare never. This leads to cryptic diagnostic messages like Type 'T' does not satisfy the constraint 'never' rather than Type '() => void' is not assignable to type ....

@gdorsi
Copy link
Contributor Author

gdorsi commented Oct 7, 2024

@milkcask

Didn't know that was possible to use the generic as argument for the extend 🤯

This opens up a possible solution for us!

I've made a quick draft, needs more tests but seems to work: https://github.com/gardencmp/jazz/pull/492/files

Screenshot 2024-10-07 at 15 02 39

@milkcask
Copy link

milkcask commented Oct 7, 2024

@gdorsi That's one of the many undocumented uses of typescript and was added because Immutable.js needed it.
See microsoft/TypeScript#25379.

As seen in that issue, it can be combined with recursion but there isn't a straightforward way to do it with plain interfaces & constraints (unlike ImmutableMap, where you still need to manually create an instance). So I guess your solution is sufficient for most use cases.

Quite an interesting problem.

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 a pull request may close this issue.

3 participants