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

RFC: Custom type-safe directives #385

Open
GauBen opened this issue Aug 29, 2024 · 4 comments
Open

RFC: Custom type-safe directives #385

GauBen opened this issue Aug 29, 2024 · 4 comments
Labels
future 🔮 An enhancement or feature proposal that will be addressed after the next release

Comments

@GauBen
Copy link
Contributor

GauBen commented Aug 29, 2024

Summary

Hi!

I'm building a GraphQL client leveraging the power of gql.tada (it's really mighty!), and I would like to implement a @paginate directive to enable dynamic pagination following relay specification.

query {
  me {
    friends(first: 10) @paginate {
      pageInfo { hasNextPage endCursor }
      edges { node { id } } 
    }
  }
}

I'd like this directive to be typed, i.e. its effects are visible in the query output type.

(The underlying implementation of the directive is for me to build)

Proposed Solution

I'd like to declare the @paginate directive in initGraphQLTada so that the result type of the query is augmented with the directive:

const graphql = initGraphQLTada<{
  // ...
  directives: {
    paginate: <Output extends { pageInfo: {}; edges: Array<{}> }>(output: Output) => (
      Output & { paginated: Reactive<Output> }
    )
  }
}>()

// The request output type would be:
type Q = {
  me: {
    friends: {
      pageInfo: { hasNextPage: boolean; endCursor: string };
      edges: Array<{ node: { id: string } }>;
      paginated: Reactive<{
        pageInfo: { hasNextPage: boolean; endCursor: string };
        edges: Array<{ node: { id: string } }>
      }>
    }
  }
} 

I've been trying to build a prototype of this, but I can't find a way to properly instantiate the generic parameter of directives['paginate']. Maybe this feature requires advanced black magic, I'll keep digging.

Requirements

None?

@GauBen GauBen added the future 🔮 An enhancement or feature proposal that will be addressed after the next release label Aug 29, 2024
@kitten
Copy link
Member

kitten commented Aug 29, 2024

Just to add this as a side-note for arguments on directives themselves to be typed or rather for GraphQLSP to recognise them, the schema that's passed to GraphQLSP/gql.tada has to be modified.

That said, regarding result types per directives, I strongly disagree with supporting this, sorry 😅 By spec and by convention, directives should never alter the return types of fields they're being put on.

We've reached a similar conclusion for @urql/exchange-graphcache, where client-side directives are supported (and automatically filtered by them having a _ prefix). The result type, while it can be modified, should never be modified.

This is even more important because, if we did alter the return type, the selection and the schema wouldn't be accurate any longer. A GraphQL schema/resolve/API can by following the spec never actually go against this either.

While it may seem for Reactive to be an exception, imho, it makes a lot more sense to have a wrapper for this, or something on the actual usage site, that either:

  • enforces that a certain field is "reactive" (depending on what that means in the context of your app/project)
  • enforces that an entire result type is altered so the Reactive type doesn't have to only be applied to a field

@GauBen
Copy link
Contributor Author

GauBen commented Aug 30, 2024

Thanks for your quick reply!

Having GraphQL-LSP to recognize custom client-side directives is as simple as that:

# .graphqlrc.yml
schema: [schema.gql, client-directives.gql]
# client-directives.gql
directive @paginate(asc: Boolean = True) on FIELD

By spec and by convention, directives should never alter the return types of fields they're being put on.

Depending on your exact definitions, you can consider than most directives actually alter the return types of the fields they're being put on:

# schema.gql
type Query {
  resolver: String!
  slowResolver: String!
}

# query.gql
query {
  resolver @skip(if: true)
  ... @defer {
    slowResolver
  }
}

For this query, without directives, I'd expect the output type to be

type Output = {
  resolver: string;
  slowResolver: string;
}

Yet gql.tada rightfully produces the following type:

type Output = {
  resolver?: string | undefined; 
  slowResolver?: string | undefined;
}

if we did alter the return type, the selection and the schema wouldn't be accurate any longer

100% agree on this and it leads me to a slightly better idea. So far, for all the use cases I have, it turns out I don't need gql.tada to output my actual runtime types, I just need it to annotate that a certain directive was put at a certain place.

For instance, back with @paginate:

query {
  me {
    friends(first: 10) @paginate {
      pageInfo { hasNextPage endCursor }
      edges { node { id } } 
    }
  }
}

Would it be possible to have gql.tada output the following type?

type Output = {
  me {
    friends: {
      pageInfo: { hasNextPage: boolean; endCursor: string | null };
      edges: Array<{ node: { id: string } }>;
      '@paginate': true;
      // or `[DirectivesSymbol]: ['@paginate']` with `const DirectivesSymbol = Symbol()`
      // or something else that cannot clash with actual runtime types
    }
  }
}

Then, to use it with a frontend framework, I'd write the following code

import { paginate } from 'somewhere';

export let data: Output;

const friends = paginate(data.me.friends);

// friends now has additional runtime features:
// - friends.loadMore() to load more pages
// - friends.isLoading() to know if a page is loading
// ... without compromising type-safety! The type of `data.me.friends` reflects
// what the server produces, but under-the-hood, the GraphQL client attached
// additional hidden data to it; it would look roughly like this:
data.me.friends === {
  pageInfo: { hasNextPage: false, endCursor: null },
  edges: [{ node: { id: 'hello' } }],
  [PaginationSymbol]: {
    replayQuery: document,
    // ...
  }
}

Thanks for your time

@kitten
Copy link
Member

kitten commented Aug 31, 2024

In your specific example, what stops you from matching the type on { pageInfo: PageInfoLike, edges: readonly EdgeLike[] }, for example?

Since the directive needs to annotate a field and the data needs to match a certain shape, you could also match on the shape and do an additional runtime check that the data is known, which is likely where the runtime value check is coming in?

That said, my concern here is mostly type inference performance. It's already not cheap to parse GraphQL documents in TypeScript's type system (hence, turbo output exists to make sure that cost doesn't balloon)

What I think is a bit more interesting is fragment-level annotations, as those are potentially much cheaper. So, I could imagine this:

const FriendPagination = graphql(`
  fragment FriendsPagination on FriendsConnection @paginate {
    pageInfo { hasNextPage endCursor }
    edges { node { id } } 
  }
`)

since that potentially reduces the cost.
That also means that you could leverage the fragment itself at the call site to enforce unwrapping and that the corresponding fragment is known, e.g. paginate(FriendPagination, data)

This solves the cost problem, since we don't have to:

  1. get the directives per field
  2. pass them on when the child is a selection
  3. intersect the directives and map them to an object-like on each selection / clean up empty directives / filter directives for known values

The benefit I see here is that we're never left with a redundant symbol field per selection check, or a type that reflects this, when directives are unknown (i.e. not part of the supported/known set of directives)

The only thing I'm unsure of is whether we should enforce underscore-syntax, which is something we've been pushing for in @urql/exchange-graphcache, where underscore-prefixed directives are automatically assumed to be "private" i.e. not sent to the API.

@GauBen
Copy link
Contributor Author

GauBen commented Sep 3, 2024

In your specific example, what stops you from matching the type on { pageInfo: PageInfoLike, edges: readonly EdgeLike[] }, for example?

A bit of ergonomics, the @paginate directive could be in charge of adding pageInfo { hasNextPage endCursor } to the query, so that the developer only has to query the data they use, and let the framework handle the boring stuff. Other than that, absolutely nothing, I thought about it too, and this is a nice and light way to handle this.

What I think is a bit more interesting is fragment-level annotations

I really like your proposal, let me make some experiments and get back to you in a few days. Thank you very much for your tips

The only thing I'm unsure of is whether we should enforce underscore-syntax

This seems like a nice and sensible convention, +1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
future 🔮 An enhancement or feature proposal that will be addressed after the next release
Projects
None yet
Development

No branches or pull requests

2 participants