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

Simplify Typings #304

Closed
panoply opened this issue Nov 13, 2024 · 4 comments · Fixed by #305
Closed

Simplify Typings #304

panoply opened this issue Nov 13, 2024 · 4 comments · Fixed by #305

Comments

@panoply
Copy link

panoply commented Nov 13, 2024

The current type system setup is unnecessarily complex and problematic. Given the expectation of an index signature and rather extraneous naming conventions, using TypeScript (TS) often results in a mild headache, requiring developers to meticulously code just to meet the driver's expectations. In most projects, interface is the preferred for type definition, with type aliases generally reserved for union or intersection types.

The driver's assumption of type alias expressions leads to cumbersome workarounds. Consider the following example:

interface Foo {
  a: string;
  b: boolean;
  c?: {
    d: string[]
  }
}

This interface is straightforward and should ideally integrate seamlessly with the driver's response model. However, the reality is less ideal:

const response = await client.query<Foo>(fql`...`);

Error Screenshot

This setup results in errors because the QueryValue constraint isn't met by our Foo interface. Even when we adjust as follows:

const response: QuerySuccess<Foo> = await client.query<Foo>(fql`...`);

The compiler still complains due to the constraints on QueryValue. The solution often involves extending the interface:

interface Foo extends Document {
  a: string;
  b: boolean;
  c?: {
    d: string[]
  }
}

const response: QuerySuccess<Foo> = await client.query<Foo>(fql`...`);

This works but introduces new issues:

  1. Assumption of Use: It assumes Foo is exclusively for query usage, which might not always be the case.

  2. Type Narrowing: Extending Document necessitates creating yet another type for accurate narrowing:

    interface FooResponse extends Foo, Document {}

This becomes problematic because:

  • The QuerySuccess type exposes a data property, making index access types less viable without creating an additional interface.
  • Using the original Foo interface elsewhere might lead to type conflicts due to the additional properties from Document.

I don't really understand why such exhaustive measures need to take place. What should be a simple generic type parameter has turned into:

  1. Creating an additional interface to extend Document.
  2. Modifying the function signature with the type parameter.
  3. Using explicit type annotations.
  4. Employing a generic type alias.

This process overly complicated and it's not necessary, furthermore what is the reasoning here? The generic can simply be augmented and resolve, there is no need for all this.

@ptpaterson
Copy link
Contributor

ptpaterson commented Nov 13, 2024

Hi @panoply Thanks for submitting the issue!

I think this can be distilled down to: Using interfaces for query responses is not well supported.

I can confirm the issue. You can add [key: string]: QueryValue as a field to your interface as a workaround as well.

As for background, we want to make it clear that the driver only decodes results into a narrow set of results, particularly that it doesn't automatically decode into custom classes. For example, if you had a custom class Foo {...}, it should always be invalid to provide the class as a query value. That said, interfaces should be fine, and we will take a look at how to fix.

@panoply
Copy link
Author

panoply commented Nov 13, 2024

FWIW: You can type check against classes.

@ptpaterson
Copy link
Contributor

ptpaterson commented Nov 13, 2024

Fauna does not decode responses into user classes, so user classes cannot be a valid QueryValue and should not be used as the type for QueryValues.

The reason we use type for QueryValue is so we can define it as a union, which you cannot do with an interface. To that end, QueryValue is a very accurate type:

export type QueryValue =
// plain javascript values
| null
| string
| number
| bigint
| boolean
| QueryValueObject
| Array<QueryValue>
| Uint8Array
// client-provided classes
| DateStub
| TimeStub
| Module
| Document
| DocumentReference
| NamedDocument
| NamedDocumentReference
| NullDocument
| Page<QueryValue>
| EmbeddedSet
| StreamToken;

I am still working through why interface Foo {} doesn't work while type Foo = {}; does.

Changing QueryValueObject to an interface doesn't fix anything on its own, because how we use them is indistinguishable between type and interface. However, if we change it to an interface that means you would be able to extend it in your own interfaces -- that's effectively the same as adding [key: string]: QueryValue to your own interface. I'll put that together and review with our team.

@ptpaterson
Copy link
Contributor

ptpaterson commented Nov 13, 2024

Okay, fascinating...
This modified example from our README compiles fine without any changes to the library

    interface User {
      name: string;
      email: string;
    }
    
    const query = fql`{
      name: "Alice",
      email: "alice@site.example",
    }`;
    
    const response = await client.query<User>(query);

But if I update the interface with property values besides string, then it fails with Index signature for type 'string' is missing in type 'User'..

    interface User {
      name: number;
      email: string;
    }

But only the FIRST property. The following also compiles

    interface User {
      name: string;
      email: number;
    }

So this seems most likely a bug with typescript (I'm using typescript v5.6).

Actually, the name property is being picked up from some of the other types, so this behavior is just a coincidence.

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.

2 participants