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

Add backwards compatible generics support for fql statements #277

Merged
merged 9 commits into from
Jul 25, 2024

Conversation

ecooper
Copy link
Contributor

@ecooper ecooper commented Jul 22, 2024

Ticket(s): FE-5389

Problem

Today, fql statements return a Query type. We'd like for fql statements to carry generic types that describe the resulting value once it's evaluated. This would allow users to "type" a fql statement once and have it be inferred in client methods. This would eliminate the need to type client call methods.

We want to do this in a backwards compatible way.

Solution

By adding a type parameter that extends QueryValue to fql statements and Query, we can enable types to be inferred in client method calls with minimal changes. So something like this "just works":

const query = fql<User>`{
  name: "Alice",
  email: "[email protected]",
}`;

// response will be typed as QuerySuccess<User>
const response = await client.query(query);

// userDoc will be automatically inferred as User
const userDoc = response.data;

Result

The following PR adds support for generics in fql statements and Query in a backwards compatible way. The following usages work:

// Use fql to type client return values...
const query = fql<User>`{
  name: "Alice",
  email: "[email protected]",
}`;

// response will be typed as QuerySuccess<User>
const response = await client.query(query);

// userDoc will be automatically inferred as User
const userDoc = response.data;

Similarly, you can still type client calls directly:

// Don't type fql
const query = fql`{
  name: "Alice",
  email: "[email protected]",
}`;

// response will be typed as QuerySuccess<User>
const response = await client.query<User>(query);

// userDoc will be User
const userDoc = response.data;

Thanks to the efforts of @ptpaterson, users are protected from conflicting types when they attempt to type both fql and client methods. See the following test for this in action:

  it("allows customers to use subtyped queries", async () => {
    const query = fql<string>`"hello"`;

    const result = (await client.query<string | number>(query)).data;
    expect(result).toEqual("hello");

    // And make sure that the opposite is not possible
    const query2 = fql<string | number>`"hello"`;
    // @ts-expect-error Argument of type 'Query<string | number>' is not assignable to parameter of type 'Query<string>'.
    await client.query<string>(query2);

Out of scope

  • Backwards incompatible changes
  • Ability to compose types across multiple fql statements
  • Large-scale type rewrites--especially since Query is publicly exported

Testing

To test, the existing query-typing tests were expanded to demonstrate fql types.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@freels freels requested a review from ptpaterson July 22, 2024 23:42
Copy link
Contributor

@ptpaterson ptpaterson left a comment

Choose a reason for hiding this comment

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

The type for Client.paginate() needs a tweek. I've a change coming for this.

Also, do we want to include Client.stream() in this PR, too?

src/client.ts Outdated Show resolved Hide resolved
src/query-builder.ts Outdated Show resolved Hide resolved
__tests__/integration/query-typings.test.ts Outdated Show resolved Hide resolved
@ptpaterson ptpaterson marked this pull request as ready for review July 25, 2024 13:59
@ecooper ecooper requested review from echo-bravo-yahoo, mwilde345, henryfauna and ptpaterson and removed request for ptpaterson July 25, 2024 16:23
@ecooper ecooper dismissed ptpaterson’s stale review July 25, 2024 19:56

ptpaterson is a contributor on the PR and resolved the open issues.

@ecooper ecooper merged commit e690352 into main Jul 25, 2024
6 checks passed
@ecooper ecooper deleted the FE-5389-fql-generics branch July 25, 2024 19:56
@@ -269,9 +270,11 @@ export class Client {

/**
* Initialize a streaming request to Fauna
* @typeParam T - The expected type of the response from Fauna. T can be inferred
* if theprovided query used a type parameter.
Copy link
Contributor

Choose a reason for hiding this comment

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

/s/theprovided/the provided/

Comment on lines +78 to +79
// @ts-expect-error Argument of type 'Query<string | number>' is not assignable to parameter of type 'Query<string>'.
await client.query<string>(query2);
Copy link
Contributor

Choose a reason for hiding this comment

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

wow cool assertion here.

Copy link
Contributor

@cleve-fauna cleve-fauna Jul 29, 2024

Choose a reason for hiding this comment

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

Would you mind adding how to use this to the development section of the README?

As we tweak type arguments in future work capturing that we can sanity check our types using this and proper linter settings will help out future devs.

For example, we got some work we'd like to do to make less boilerplate for users when definining their own types and when using .query calls that return pages (as opposed to using the clients pagination utils - there's cases you might do that in an API for example). We have some of these type improvements documented internally here: https://faunadb.atlassian.net/wiki/spaces/DX/pages/3852697604/Developer+Tooling+Pain#API-Pagination

So this test pattern is something we will likely need aagain.

Just linking here: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-9.html with some text about the linter settings needed would suffice.

Copy link
Contributor

@cleve-fauna cleve-fauna left a comment

Choose a reason for hiding this comment

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

Looks good! I like the solution. Found one small doc typoe.

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.

4 participants