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

Masked Fragment inference with Remix/React Router useLoaderData pattern #110

Closed
3 tasks done
benjamindulau opened this issue Mar 6, 2024 · 6 comments · Fixed by #126
Closed
3 tasks done

Masked Fragment inference with Remix/React Router useLoaderData pattern #110

benjamindulau opened this issue Mar 6, 2024 · 6 comments · Fixed by #126
Labels
documentation 📖 This needs to be documented but won't need code changes

Comments

@benjamindulau
Copy link

Describe the bug

I'm trying to migrate from codegen to gql.tada but TypeScript complains about types not being compatible when data comes from a Remix or React Router loader.

This may be related to how TypeScript handles union types when a function can return different types... But I'm not sure.

As you can see, I reproduced a case here https://stackblitz.com/edit/remix-run-remix-rckuw2?file=app%2Froutes%2Ftada%2Froute.tsx

TypeScript complains when using the result of useLoaderData<typeof loader>(). As far as I'm concerned they should be compatible though.

You can see that there is no issue when using useQuery inside the component's body.

And if you look in the routes/codegen directory, you can see that I re-created the exact same use case but with codegen and it seems to go along with this without any issue.

What is the difference between codegen and gql.tada approach here?
This is clearly a blocker for us because we don't want to be forced to redeclare every loader return types manually as this wouldn't be a very good DX.

Reproduction

https://stackblitz.com/edit/remix-run-remix-rckuw2?file=app%2Froutes%2Ftada%2Froute.tsx

gql.tada version

gql.data 1.2.1

Validations

  • I can confirm that this is a bug report, and not a feature request, RFC, question, or discussion, for which GitHub Discussions should be used
  • Read the docs.
  • Follow our Code of Conduct
@kitten
Copy link
Member

kitten commented Mar 6, 2024

The issue here seems to be that useLoaderData maps the object (potentially recursively?) inside JsonifyObject.

I don't know what that type does and haven't looked at it. But if I had to guess, it's meant to prevent you from trying to transfer non-JSON values over loaders, since that wouldn't be possible.

I'm not sure if they have an escape hatch for that, as otherwise you'd be forced to unmask fragments (or disable masking entirely). But since masked fragments are a type-level protection for us, it's probably advisable to simply turn off / circumvent the JsonifyObject mapping, since GraphQL already guarantees that transferred objects are JSON-serializable by default (unless you were to add custom scalar types / type mapping, etc)

The easiest way to circumvent this, if there's no built-in solution for this, is to write a wrapper for useLoaderData, specifically for GraphQL data, that casts the return type, so JsonifyObject doesn't come into play.

@kitten kitten changed the title Weird TypeScript issue when using with Remix/React Router loaders (but no issue with codegen) Masked Fragment inference with Remix/React Router useLoaderData pattern Mar 6, 2024
@davidesigner
Copy link

Hi @kitten, thanks for your reply.

How do you explain that the JsonifyObject works with Codegen but not with Tada?

@kitten
Copy link
Member

kitten commented Mar 8, 2024

Depending on what you're using for codegen it first of all depends what you did before. However, assuming you're using GraphQL Code Generator with the client-preset, its types generate similar “ref types” for fragment masks, but they're instead placing it on a regular property (https://github.com/dotansimha/graphql-code-generator/blob/b36509997251d1e3538634addbc52d0e87064113/packages/presets/client/src/fragment-masking-plugin.ts#L8-L13)

This basically means:

  • if you're using another code generator that doesn't use fragment masking, this is not an issue, because the types will always match the JSON result exactly
  • if you're using the client-preset (or codegen that otherwise generates fragment masks), their types may use serializable properties (e.g. ' $fragmentRefs')

This is actually intended behaviour, in a way.

We're choosing to use a unique symbol exactly because fragment masks are neither real nor serializable. Once you have a fragment mask, the data basically “lies” about what its shape is and omits information rather than showing it.

So, you can either disable fragment masking, and lose its benefits, enforcing fragment composition strictly, or you'll have to avoid going through JsonifyObject, or write wrappers to retain fragment masks.

On a side note, Remix is definitely doing the right thing here, because they're basically enforcing that unserializable data can never make it across, since it's presumably serialized as JSON. But this being a virtual property, rather than omitting it, their helper makes it incompatible, since they likely only expect this to be a problem in case of any mistakes.

@kitten kitten added the documentation 📖 This needs to be documented but won't need code changes label Mar 8, 2024
@kitten
Copy link
Member

kitten commented Mar 11, 2024

I found that #126 happens to fix this, since JsonifyObject does not filter keys, so while this wasn't intended to address this, this should eliminate any boilerplate code you'd have to apply yourself to map over types.

@davidesigner
Copy link

Wow, we look forward to try it out with the next version. Thanks.

@davidesigner
Copy link

I confirm that is fixed. Thanks Kitten!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 📖 This needs to be documented but won't need code changes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants