feat: generated auto-typed hooks

[tgriesser]

Hello 👋! First contribution here and it's a decently large diff (edit: split the example app out to #307), so I figured it might be good to start with a little background on the "why" for this proposed addition to the compiler.

Prior to Relay, I had mostly used Apollo and Urql, with graphql-code-generator to code generate operations into named hooks, which wrap the primitive hooks (useQuery, useMutation, etc.) but are fully type-safe without any additional imports or need to fill generics.

query ViewerQuery {
  id
  name
  ...ProfileImage
}

const { data, loading } = useViewerQuery() // auto-generated / correctly typed

This is the one thing I really miss when working with Relay.

The origin of this change came as I've been toying around with a Relay Next.js integration, using a dedicated set of wrapper hooks for simplifying page-level routing and handling the conditional rendering needed server and client side (hopefully will get some time to open source soon).

I was using this TypeScript compiler project to code-generate these new next-specific hooks, and realized that a lot of what I was doing there might also be useful in the "official" TS compiler, rather than maintaining/marketing a fork. So I extracted out what I thought would be useful in the core, and here it is!

Goals / Problems Solved:

No change in existing functionality

This is opt-in, via a different entry-point --language typescript/hooks adds the hook wrappers along with the currently generated artifact, for example:

mutation TodoListFooterRemoveCompletedTodosMutation(
  $input: RemoveCompletedTodosInput!
) {
  removeCompletedTodos(input: $input) {
    deletedTodoIds
  }
}

adds:

export const useTodoListFooterRemoveCompletedTodosMutation = (mutationConfig?: (environment: IEnvironment, config: MutationConfig<TodoListFooterRemoveCompletedTodosMutation>) => Disposable) => {
  return useMutation<TodoListFooterRemoveCompletedTodosMutation>(node, mutationConfig)
}

at the bottom of the file after the node. Aiming to keep the same signatures as the normal hooks, less the node and the generic slots, which we pre-fill.

Properly typed fragments:

This one is subtle, but I've found to be a common pain-point/source of confusion, given the prominence of fragments in Relay. Because the typing is often inferred based on the typing of the fragmentRef argument, it's possible to pass an incorrect identifier for the fragment you're looking to fulfill.

This should be an error, but it's not. Instead the error will be detected elsewhere when the incorrect returned value is used, which is not ideal.

With the changes in this PR:

query ViewerQuery {
  id
  name
  ...ProfileImage
}

const viewer = useViewerQuery()

const profileImage = useProfileImageFragment(viewer) // properly type checked

Improved DX when renaming fragments or operations

This is the biggest pain point/source of friction for me with Relay. Because the naming of the operations corresponds directly to the module they are declared in, renaming a fragment or operation can be pretty frequent, and requires updating not just the import, but the typings for those import. By wrapping this all with a hook, you have less types to import and generic slots to fill / rename, leading to a better overall experience when iterating.

Detect when arguments aren't required, and make them optional

A lot of times, particularly on root level queries, you wont have any variables, but the typings of the relay hooks make this required. Because we're generating the hooks from the AST metadata, we can know ahead of time if there are arguments, and adjust the typings accordingly

Simplified ergonomics on refetchable / connection directives

One of the best things about Relay IMO is the patterns around refetchable fragments & connections. They are also the most cumbersome to type, because they require filling two generic slots. Again, with the metadata from the compiler we can know how to type these properly, so all you need to worry about is importing the usePaginated{...} hook

---

I decided to create a new example TodoApp for this feature (see #307), aiming to demonstrate some of the example real-world uses of the generated loading / preloading, refetchable fragments, paginated fragments, mutations, etc. removing some of the optimistic update boilerplate which IMO is a little overkill for most use-cases.

Interested in any feedback (and feel free to jump in with any edit commits, I did it pretty quickly so it's probably a little rough). And if it turns out this would be better off as a fork/separate compiler that's cool too.

[tgriesser]

Just published this under https://www.npmjs.com/package/relay-compiler-language-typescript-hooks-gen-preview if anyone wants to try it out and give feedback just:

• Ensure you are using relay@11
• yarn add --dev relay-compiler-language-typescript-hooks-gen-preview
• Replace --language typescript with --language typescript-hooks-gen-preview

[zephraph]

Based on this I'm assuming this change will likely need to be apart of a major version bump.

[n1ru4l]

we can ship this and the remaining breaking changes I have in my branch as a new major version next month while also dropping node 10

[zephraph]

That's true, yeah. Given that there's an extra entry point (a sort of opt-in if you will) I feel like we could likely loosen this requirement with the caveat of course that if you're using hooks version you'll need to be on v11. Not sure if it's worth encoding that as a dependency or runtime constraint vs just documenting it.

[zephraph]

Can it be deeply imported? https://github.com/DefinitelyTyped/DefinitelyTyped/blob/fc5c33122aa1167f3dbbd7f74ef6a6d5a663f9cb/types/react-relay/relay-hooks/usePaginationFragment.d.ts#L6

[tgriesser]

Those DefinitelyTyped files are lying a bit 😆 - I suppose we could do

import type { usePaginationFragmentHookType } from 'react-relay/relay-hooks/usePaginationFragment

but that's technically not a valid import path from the js.

[zephraph]

Ah, yep. Makes sense.

[zephraph]

I really love this. While we haven't yet started using relay hooks at @artsy, I'm hopeful that's on the horizon and this could be a huge QoL improvement. I'll lean on @alloy a bit to dig a bit deeper, but I'll help out where I can.

There's a few things I think we should improve on before merging:

• The hooks module is pretty standalone at the moment and re-implements rather than reuses some of the base utilities. I'd like to massage this until it feels less one off and more of an extension of the api.
• We should definitely add tests, but that can wait until we're closer to the end.
• I'd like to consider softening the peer dependency requirements. I understand react-relay has to be v11+ to use hooks, but it'd be nice to be able to maintain compatibility for those not yet on them. Admittedly, I'm not 100% on this idea yet and would love some thoughts.

[zephraph]

derivedFrom is defined from Relay's split operation transformer that runs as a part of it's overall query transform operation. It's either the name of the node the referenced node is derivedFrom or undefined.

So, if it's derived no hooks are generated.

I get all that. I don't think I understand when it's classified as derived vs not. The transform looks like it runs on all operations. I guess only the base most nodes would be considered not derived?

[tgriesser]

IIRC I believe that this is was for refetchable fragments: https://github.com/facebook/relay/blob/d59a32bcdbc11341f972def8e5967408a2131c18/packages/relay-compiler/language/javascript/RelayFlowGenerator.js#L989-L1006 essentially if it's "derived" then it's the operation name that will be called when refetching, but we don't actually want to create a use{...}Query definition for it because that's already encapsulated in the fragment.

From the example app:

fragment TodoAppData on User
  @refetchable(queryName: "TodoAppRefetchQuery")
  @argumentDefinitions(
    last: { type: "Int" }
    first: { type: "Int" }
    after: { type: "String" }
    before: { type: "String" }
  ) {
    id
    totalCount
    isAppending
    ...TodoListFooterData
    ...TodoList
      @arguments(last: $last, first: $first, after: $after, before: $before)
}

We don't actually want to generate a useTodoAppRefetchQuery hook... that's just an internal implementation detail of the const [data, refetch] = useTodoAppData(data) fragment hook.

And that's why nothing is output at the bottom of this file here:

https://github.com/tgriesser/relay-compiler-language-typescript/blob/c18a58f2cd2f82bb0c6f5b5bc6875779ff9d33e8/example-hooks-gen/ts/__relay_artifacts__/TodoAppRefetchQuery.graphql.ts#L410-L422

[zephraph]

Cool, thanks!

[ds300]

This is extremely cool! 🎉 Thanks for sharing your work @tgriesser

I have some thoughts about DX:

In terms of making it easier to write relay components this is an obvious huge win 🙌 . My only concern is around readability. I found it quite frustrating to read the Todo example app because navigating from a hook usage to the corresponding source graphql tag required manual searching. I suppose a team could align on a convention of keeping the graphql tag next to the hook if they care about that, but personally I'd prefer to have navigability enforced through the API somehow.

One option might be to make people provide a reference to the node despite that fact that it's not required.

const userFragment = graphql`
  fragment MyAccountUser on User {
    name
  }
`

const MyAccount = (props) => {
  const user = useMyAccountUser(userFragment, props.user)
}

A small price to pay for cmd+click IMO, but I can see why others would prefer the proposed API :)

Of course this could also be a configurable option. Or maybe you can think of another way to ensure navigability?

[tgriesser]

This is extremely cool! 🎉 Thanks for sharing your work @tgriesser

Thanks!

My only concern is around readability. I found it quite frustrating to read the Todo example app because navigating from a hook usage to the corresponding source graphql tag required manual searching.

The Todo App is admittedly in bad shape because I didn't upgrade the relay babel compiler and so the 1 operation per graphql tag isn't enforced (so I just threw them all in one tag), which is apparently no-longer permitted 😄. I wonder if moving them closer to their use as you point out is a better option.

I suppose a team could align on a convention of keeping the graphql tag next to the hook if they care about that, but personally I'd prefer to have navigability enforced through the API somehow.

Is this mostly a concern for fragments, or for operations as well? I can take a look and see how bad it might be to add overloads for that. I think it'd be possible to do, but I'd almost think that if explicitness is a concern/goal, then just using the useFragment API might be the route to go there.

Or maybe you can think of another way to ensure navigability?

For me personally it's not a huge concern, because at the very least you should be guaranteed to have the template literal co-located somewhere in the file you're currently in, so it's probably just a few keystrokes to search around and find it no matter the editor.

One other thing that maybe? would be nice is if we take the docstring https://github.com/tgriesser/relay-compiler-language-typescript/blob/c18a58f2cd2f82bb0c6f5b5bc6875779ff9d33e8/example-hooks-gen/ts/__relay_artifacts__/TodoAppAddTodoMutation.graphql.ts#L41-L71 output in the file and moved it over the hook? Not sure

[tgriesser]

re: @zephraph

The hooks module is pretty standalone at the moment and re-implements rather than reuses some of the base utilities. I'd like to massage this until it feels less one off and more of an extension of the api.

Good call!

We should definitely add tests, but that can wait until we're closer to the end.

Totally missed that there was even a tests directory 🙃

I'd like to consider softening the peer dependency requirements. I understand react-relay has to be v11+ to use hooks, but it'd be nice to be able to maintain compatibility for those not yet on them. Admittedly, I'm not 100% on this idea yet and would love some thoughts.

Yeah good point, assuming this is kept as a separate entry point we could easily do a semver check when you try to use it and throw if the peerDep is incorrect

[zephraph]

we could easily do a semver check when you try to use it

I'm largely in favor of that.

[alloy]

This is amazing, thanks @tgriesser!

I think this would be great to have. Aside from the various people already reviewing the pieces in this PR, I want to be mindful of how far we would diverge from upstream Flow based Relay, and if that is a problem at all.

@josephsavona @kassens @jstejada et al, do you have thoughts on the changes here and considering something like it for upstream or otherwise a divergence, which is a bit more complicated due to the new compiler no longer allowing for a plugin like this?

[alloy]

Properly typed fragments:

This one is subtle, but I've found to be a common pain-point/source of confusion, given the prominence of fragments in Relay. Because the typing is often inferred based on the typing of the fragmentRef argument, it's possible to pass an incorrect identifier for the fragment you're looking to fulfill.

@tgriesser Great point, I was recently thinking about this as well and was actually wanting to see if we could leverage TypeScript’s Template Literal Types feature to infer the fragment name and match it against the given reference. That would mean useFragment itself would be safe.

However, as I don’t believe Flow has this ability, that would again be a divergence, but at least a very slight one that would make use of available language features.

Here’s an example, but alas it looks like TS currently cannot infer from the argument to a tag function. Will ask around.

[jstejada]

I haven't fully read through this pr, but just to add some context:

the way we guarantee that the right $key type fragment ref is passed to the correct useFragment call is via a lint rule: https://github.com/relayjs/eslint-plugin-relay/blob/master/src/rule-generated-flow-types.js

looks like this was also suggested here: relay-tools/tslint-plugin-relay#15

I'm inclined to say that if we're able to achieve what @alloy is showing in the example could be a good way to guarantee the type safety we want without additional codegen.

as @kassens mentioned in our chats, we see defining queries and fragments in the files that use the data requested there as a pretty critical piece of Relay. This is what allows product developers to hopefully reason pretty locally about a component.

So ideally if we can rely solely on the type system to achieve this level of safety that would be great.

As an example that @kassens suggested when discussing this, if we could get the type of the graphql tagged string to be the extracted type of Foo.graphql.ts, we might be able to do something like:

useFragment(graphql`fragment Foo { ... }`, x);

where the type system requires a FragmentReference<'Foo'> for type x, by extracting that string, and use FragmentReference<'Foo'> to extract the return type like we do today with the $key types.

[tgriesser]

I was recently thinking about this as well and was actually wanting to see if we could leverage TypeScript’s Template Literal Types feature to infer the fragment name and match it against the given reference. That would mean useFragment itself would be safe.

@alloy This was actually the first thing I had looked into, and I agree this would probably be a better overall approach. I think that microsoft/TypeScript#33304 is a blocker in order to allow for that.

[josephsavona]

My reaction to this is simultaneously "wow this is super cool" but also "hmm I'm not really sure if this is the direction we want".

I think this would be great to have. Aside from the various people already reviewing the pieces in this PR, I want to be mindful of how far we would diverge from upstream Flow based Relay, and if that is a problem at all.

Re direction: this approach increases code-size, which can in turn negatively affect application performance - already not great. This also feels like it could be harder to teach - note that if you aren't using Flow/TypeScript you can use useFragment() without needing to import anything at all, which is really convenient. Whereas here you always have to import a generated artifact.

Finally, there is the colocation question - it feels like this API encourages sharing of fragments in a way that useFragment doesn't. It's a subtle difference - importing a type w useFragment vs importing a function w this approach - but it does seem like it would meaningfully affect developer behavior and encourage them to do the wrong thing.

@josephsavona @kassens @jstejada et al, do you have thoughts on the changes here and considering something like it for upstream or otherwise a divergence, which is a bit more complicated due to the new compiler no longer allowing for a plugin like this?

Before we discuss how i think we should align on direction. My comments here aren't to say "we think this is definitely a bad idea" - rather, we have some serious concerns and aren't sure. I'm curious to hear feedback from folks who've used this approach in other frameworks.

[n1ru4l]

My reaction to this is simultaneously "wow this is super cool" but also "hmm I'm not really sure if this is the direction we want".

I think this would be great to have. Aside from the various people already reviewing the pieces in this PR, I want to be mindful of how far we would diverge from upstream Flow based Relay, and if that is a problem at all.

Re direction: this approach increases code-size, which can in turn negatively affect application performance - already not great. This also feels like it could be harder to teach - note that if you aren't using Flow/TypeScript you can use useFragment() without needing to import anything at all, which is really convenient. Whereas here you always have to import a generated artifact.

Finally, there is the colocation question - it feels like this API encourages sharing of fragments in a way that useFragment doesn't. It's a subtle difference - importing a type w useFragment vs importing a function w this approach - but it does seem like it would meaningfully affect developer behavior and encourage them to do the wrong thing.

@josephsavona @kassens @jstejada et al, do you have thoughts on the changes here and considering something like it for upstream or otherwise a divergence, which is a bit more complicated due to the new compiler no longer allowing for a plugin like this?

Before we discuss how i think we should align on direction. My comments here aren't to say "we think this is definitely a bad idea" - rather, we have some serious concerns and aren't sure. I'm curious to hear feedback from folks who've used this approach in other frameworks.

I think the issue with that is that there is currently no framework that implements data masking via fragments like relay.

Tools like graphql-codegen already produce this kind of typed hooks (but only for useMutation and useQuery, as other frameworks have no useFragment hook counter-part), but eventually moved on towards the typed document node approach which is now supported by apollo and urql. This allows inferring the return types of the hook from the document node passed to the hook. https://the-guild.dev/blog/typed-document-node

Maybe supporting typed document nodes might be a possible solution for relay as well?

[tgriesser]

My reaction to this is simultaneously "wow this is super cool" but also "hmm I'm not really sure if this is the direction we want".

@josephsavona Thanks for the feedback! I share this sentiment as well - not sure if this is the desired API, but was mostly just looking to spark the conversation of how we might provide sounder types with require fewer imports and generic filling. Particularly for usePaginationFragment / useRefetchableFragment, where you're importing types from both the the fragment and the refetch query.

Re direction: this approach increases code-size, which can in turn negatively affect application performance

That is true. Hopefully it's not too costly since most of the footprint is typings, and the hooks are mostly just wrapping what you'd normally be importing to use in the component anyway (except for queries, where it's currently adding all of the possible query permutations), but it's definitely a valid concern.

Finally, there is the colocation question - it feels like this API encourages sharing of fragments in a way that useFragment doesn't. It's a subtle difference - importing a type w useFragment vs importing a function w this approach - but it does seem like it would meaningfully affect developer behavior and encourage them to do the wrong thing.

I don't think I'd even considered that, but that's a good point that it could encourage/enable that sort of incorrect behavior. I thought that sort of incorrect usage was runtime checked by Relay, but I was probably thinking of something else.

Tools like graphql-codegen already produce this kind of typed hooks (but only for useMutation and useQuery, as other frameworks have no useFragment hook counter-part), but eventually moved on towards the typed document node approach which is now supported by apollo and urql. This allows inferring the return types of the hook from the document node passed to the hook. the-guild.dev/blog/typed-document-node

Maybe supporting typed document nodes might be a possible solution for relay as well?

@n1ru4l I think I missed this evolution to TypedDocumentNode - looks neat! It'd be great if there is some movement on the TemplateStringsArray typing support, so it'd be possible for something along those lines.

[alloy]

I’m checking internally [at MS] if there's a possibility to look at that sooner, but can't say anything about how successful that might be.

In the meantime, I’m wondering how feasible it would be for us to support both graphql`…` and graphql(`…`), such that we can use the latter for in TS today. Afaik it is only really looked at when extracting the document, which exists in this plugin, and the babel plugin.