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

Improve GraphQL TypedDocumentNode usage in the new MSW.js version #1881

Closed
1 task
MatejBransky opened this issue Nov 22, 2023 · 28 comments · May be fixed by #2018
Closed
1 task

Improve GraphQL TypedDocumentNode usage in the new MSW.js version #1881

MatejBransky opened this issue Nov 22, 2023 · 28 comments · May be fixed by #2018

Comments

@MatejBransky
Copy link

MatejBransky commented Nov 22, 2023

Scope

Improves an existing behavior

Compatibility

  • This is a breaking change

Feature description

Hi, we have successfully used MSW.js v1 with generated GraphQL typed documents (see type notation in screenshots) so we have to just import the TypedDocumentNode document itself and the rest has been inferred. It was one of the selling points in our team.

image

But this behavior doesn't work in the latest version because we have to return type-agnostic HttpResponse.json() which can't know what the response type should be.

image

Am I missing some new way to do it with the same comfort, or is this completely gone?

@MatejBransky
Copy link
Author

The workaround is to pass the query type explicitly. But DX is worse than the previous major version.

image

@kettanaito
Copy link
Member

Hey. This looks like a bug with the body type inference. Thanks for sharing this! A type test is clearly missing on our part.

@MatejBransky
Copy link
Author

Glad to hear that it is just a bug. Thank you for your work! You guys rock! 🚀

@pleunv
Copy link

pleunv commented Feb 1, 2024

@MatejBransky I can't reproduce this behavior myself, and I have working type inference for the HttpResponse.json({...}) object by simply passing the TypedDocumentNode document. Do you still encounter this today?

One thing worth noting is that if you're using __typename, you need an as const annotation or inference will break:

graphql.query(GetUser, () => 
  HttpResponse.json({
    __typename: 'User' as const,
   // ... user properties, with inference
  })
);

@aaronadamsCA
Copy link

@pleunv Could you please confirm that if you manually pass the query type as a type parameter, everything works as you expect it:

graphql.query<GetUserQuery>(GetUser, () => 
  HttpResponse.json({
    __typename: 'User',
   // ... user properties, with inference
  })
);

If so, that more or less confirms the same problem I'm seeing: when I call graphql.query(typedDocumentNode, resolver), TypeScript seems to infer the type of Query from resolver instead of typedDocumentNode. This would explain both the type mismatch and why as const resolves the problem.

Unfortunately my TypeScript skills are right in that sweet spot where I am pretty sure I understand the problem, but I have absolutely no idea how to solve it.

@aaronadamsCA
Copy link

aaronadamsCA commented Feb 6, 2024

Edit: I tried the below locally using pnpm patch and it didn't work, but maybe it helps point toward a solution.

Looking at this:

msw/src/core/graphql.ts

Lines 26 to 36 in e56f624

export type GraphQLRequestHandler = <
Query extends GraphQLQuery = GraphQLQuery,
Variables extends GraphQLVariables = GraphQLVariables,
>(
operationName:
| GraphQLHandlerNameSelector
| DocumentNode
| TypedDocumentNode<Query, Variables>,
resolver: GraphQLResponseResolver<Query, Variables>,
options?: RequestHandlerOptions,
) => GraphQLHandler

And given this: microsoft/TypeScript#14829 (comment)

Just an update - T & { } creates a "lower-priority" inference site for T by design. I would move this from the "definitely don't depend on this" column to the "it's probably going to work for the foreseeable future" column.

The fix might be as simple as this:

export type GraphQLRequestHandler = <
  Query extends GraphQLQuery = GraphQLQuery,
  Variables extends GraphQLVariables = GraphQLVariables,
>(
  operationName:
    | GraphQLHandlerNameSelector
    | DocumentNode
    | TypedDocumentNode<Query, Variables>,
  resolver: GraphQLResponseResolver<Query & {}, Variables & {}>,
  options?: RequestHandlerOptions,
) => GraphQLHandler

Which should hopefully encourage TypeScript to infer Query and Variables from operationName instead of resolver.

@pleunv
Copy link

pleunv commented Feb 6, 2024

@pleunv Could you please confirm that if you manually pass the query type as a type parameter, everything works as you expect it:

graphql.query<GetUserQuery>(GetUser, () => 
  HttpResponse.json({
    __typename: 'User',
   // ... user properties, with inference
  })
);

Can confirm that works as well, yeah. You have two options when using TypedDocumentNode:

  • Importing the operation data & variable types and passing them as generic type annotations to graphql.query<TData, TVars>() and graphql.mutation<TData, TVars>()
  • Marking all string literals as as const

Both are a bit annoying, I went with the latter for now :)

@kettanaito
Copy link
Member

@aaronadamsCA, thanks for the suggestion! I don't think the T & {} works how you expect. It just creates a unique type, preventing type matching for broader types. A small example to illustrate:

function accept(name: 'john' | string): void {}
// All string inputs will be "coerces" to the shared
// type—which is just string. You won't get autocompletion
// on the "john" type of the "name".
accept('john')
accept('sarah')

function accept(name: 'john' | string & {}): void {}

// By using "& {}", we tell TS to differentiate between
// the "john" base type (string) and the "any other string"
// type. As a result, we get a proper autocompletion for "john":
accept('john')

// We also get any strings as "name" supported.
accept('sarah')

It doesn't really affect the order in which the generic parameters are inferred, afafik.

@kettanaito
Copy link
Member

kettanaito commented Feb 6, 2024

I've added a type test for TypedDocumentNode and it passes without issues: #2018.

Can it be that the TypedDocumentNode type itself has changed? We don't depend on GraphQL Code Generator in MSW, instead we are just replicating the TypedDocumentNode type to provide query and variable types inference:

msw/src/core/graphql.ts

Lines 17 to 24 in e56f624

export interface TypedDocumentNode<
Result = { [key: string]: any },
Variables = { [key: string]: any },
> extends DocumentNode {
__apiType?: (variables: Variables) => Result
__resultType?: Result
__variablesType?: Variables
}

This means if the GQL Code Gen was to change this type, like swapping the order of generics or changing the return type, MSW would stop inferring from it.

This is also why the suggestion from @aaronadamsCA may work. If the TypedDocumentNode has the __typename in its return type, that will trigger the type inference. Now, trying to find where the TypedDocumentNode type is defined...

@kettanaito
Copy link
Member

Alas, just checked the TypedDocumentNode type and it remains the same: https://github.com/dotansimha/graphql-typed-document-node/blob/925c2b0483ae602eec38253acf602c2aefe383ec/packages/core/src/index.ts#L3-L10.

I've also added @graphql-typed-document-node/core to use in GraphQL type testing with TypedDocumentNode, and the inference still works as of MSW 2.1.6.

Needs help

Does anybody have a reproduction repository for this? Without it, I would have to close this issue as non-reproducible.

@aaronadamsCA
Copy link

aaronadamsCA commented Feb 6, 2024

I tried the TypeScript v5.4.0-beta which ships a new NoInfer utility type. That didn't solve the problem either, so I may have been off the mark RE: inference. It might be a contravariance problem or something? The type error isn't quite what I'd expect it to be.

@kettanaito, I would expect #2018 to pass because your typed document node expected name to be a string. Try this:

 const GetUserQuery = {} as TypedDocumentNode<
   {
     user: {
-      name: string
+      name: 'John'
     }
   },
   { userId: string }

If that doesn't reproduce, I'll see what I can put together (I tried earlier today but the CodeSandbox starter seems to be broken, the dependencies won't install).

@kettanaito
Copy link
Member

@aaronadamsCA, I've just tried a narrower type for user.name and the test still passes. The point there is that the inference was correct, so no matter what types you provide, MSW will respect hose.

I think the only case when that won't happen is if the type of TypedDocumentNode (the actual liter value) doesn't extend this type MSW expects (which is also the type that GQL Code Gen generates):

msw/src/core/graphql.ts

Lines 17 to 24 in e56f624

export interface TypedDocumentNode<
Result = { [key: string]: any },
Variables = { [key: string]: any },
> extends DocumentNode {
__apiType?: (variables: Variables) => Result
__resultType?: Result
__variablesType?: Variables
}

Can you please look at the type of your document node and post it here? What fields does it have? There must be some divergence that causes the inference to break.

Once again, a reproduction repository would be best.

@MatejBransky
Copy link
Author

MatejBransky commented Feb 6, 2024

I've created this repo: https://github.com/MatejBransky/graphql-example

Try to complete the already existing mocked query handler in the file tests/msw/handlers.ts.

The "foobar" doesn't trigger any TS error and expected "state" field should be suggested but it's not.

The suggested shape should be:

HttpResponse.json({
  data: {
    info: {
      state: 'something'
    }
  }
})

@aaronadamsCA
Copy link

@kettanaito
Copy link
Member

@MatejBransky, thanks for providing the repo! If you open src/generated/graphql.ts, you can spot a TypeScript error right here:

import { TypedDocumentNode as DocumentNode } from '@graphql-typed-document-node/core'

There are no type definitions for the @graphql-typed-document-node/core so DocumentNode will be any. Once you install that package, you get the right type inference from MSW:

pnpm i @graphql-typed-document-node/core
...
Type '{ data: { foobar: string; }; }' is not assignable to type 'GraphQLResponseBody<GetAppInfoQuery>'.
        Types of property 'data' are incompatible.
          Property 'info' is missing in type '{ foobar: string; }' but required in type 'GetAppInfoQuery'.ts(2345)

It reports foobar as unknown and also autocompletes the info key as expected.

@kettanaito
Copy link
Member

kettanaito commented Feb 6, 2024

@aaronadamsCA, I'm getting what looks like correct type validation in that sandbox as-is:

Screenshot 2024-02-06 at 16 52 33

Is the expected state different from this?

@MatejBransky
Copy link
Author

@kettanaito Awesome! Thank you! I can close it I guess?

@kettanaito
Copy link
Member

@MatejBransky, glad it helped!

Give me a moment, I think I finally understand the problem with the sandbox from @aaronadamsCA. Investigating...

@kettanaito
Copy link
Member

kettanaito commented Feb 6, 2024

@aaronadamsCA, that looks like an issue but I can't figure out where it belongs. Sharing my thoughts.

The HttpResponse.json() body generic parameter will infer the type from the given value as broadly as possible. So foo: 'BAR' gets inferred as foo: string. MSW still correctly infers the return type from the query because it's that type that causes the error:

  • The resolver is annotated to return foo: 'BAR' type while the actual response is foo: string (a more permissive type).

I can see now what you were talking about. This may be beyond my skill. Perhaps @Andarist can have a piece of advice here once he's got a minute. Here's a playground that distills this example to the minimum.

@kettanaito
Copy link
Member

kettanaito commented Feb 6, 2024

I will close this issue because it needs no changes on MSW's side. What @andreawyss is experiencing is not related to TypedDocumentNode. You can reproduce that even with a response body generic using the http handler. I will keep an eye on this thread if any suggestions come our way on how to force TypeScript to infer the type as narrowly as possible.

@MatejBransky
Copy link
Author

MatejBransky commented Feb 6, 2024

Ok, I've found one issue even after adding the missing typings - if I don't fill the expected response the TS doesn't complain about the missing type. 🤔

image

And nested fields are not suggested. 👀
image

This wasn't the issue in the v1:
image

I've pushed this example to the mentioned repo.

@aaronadamsCA
Copy link

aaronadamsCA commented Feb 6, 2024

@aaronadamsCA, that looks like an issue but I can't figure out where it belongs. Sharing my thoughts.

The HttpResponse.json() body generic parameter will infer the type from the given value as broadly as possible. So foo: 'BAR' gets inferred as foo: string. MSW still correctly infers the return type from the query because it's that type that causes the error:

  • The resolver is annotated to return foo: 'BAR' type while the actual response is foo: string (a more permissive type).

Yep, you've got it, @kettanaito. Therefore it happens with every __typename and with every enum in the schema.

I can see now what you were talking about. This may be beyond my skill. Perhaps @Andarist can have a piece of advice here once he's got a minute. Here's a playground that distills this example to the minimum.

I'm not entirely sure the solution involves preventing type widening (which I don't think is possible?), but rather in controlling type inference. There must be a way to tell TypeScript to infer the query type from operationName and then apply it to resolver. I just have no idea how.

@kettanaito
Copy link
Member

@aaronadamsCA, to clarify, this part is working as you describe:

There must be a way to tell TypeScript to infer the query type from operationName and then apply it to resolver

It infers your query and variable types and annotates the resolver's return type to match the query type. There are no issues with this.

The issue is the inference here:

HttpResponse.json<T>(body: T)

What HttpResponse.json() returns must be compatible with what the response return type has inferred. It is during the json body inference that the given type is inferred as broadly as possible.

I don't believe there's a way for the HttpResponse.json() to infer type from the resolver surrounding it. There may be TS magic to do this but I don't posses that incantation.

@aaronadamsCA
Copy link

aaronadamsCA commented Feb 6, 2024

I've created a new reproduction that's as short as possible:

https://stackblitz.com/edit/msw-response-type-mismatch?file=index.ts

I think this is the best illustration that there really is a bug here, because it can be worked around by explicitly passing the same type parameter that is already being inferred. You can see that in both cases the type parameters to graphql.query are the same, yet one works and one doesn't.

I don't know, maybe it's a TypeScript issue.

Edit: Ah, simultaneous posts. I understand what you're saying, but then I would expect the workaround to fail as well. You can see that the inferred type of HttpResponse.json() differs between badHandler and goodHandler—for no apparent reason, really.

@kettanaito
Copy link
Member

@aaronadamsCA, may I please ask you to open a new issue about this? I would like to take a proper look at it. It's just not related to TypedDocumentNode, really. Thanks.

@MatejBransky
Copy link
Author

MatejBransky commented Feb 7, 2024

So just to clarify - does that mean that in MSW v1 was possible to infer the return type from the typed document node immediately without any __typename: 'typename' as const and in v2 you have to add that field in order to get the same type hint?

@Andarist
Copy link
Contributor

Andarist commented Feb 7, 2024

The easiest way to enforce const-like inference is to use const type parameters that were introduced in TS 5.0:
TS playground

@MatejBransky
Copy link
Author

Ok, so the workaround/solution is in the comment of the related PR. Thanks @pleunv ! 🙏

@github-actions github-actions bot locked and limited conversation to collaborators Oct 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants