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

Type error when graphql.query infers type arguments from TypedDocumentNode #2023

Open
4 tasks done
aaronadamsCA opened this issue Feb 7, 2024 · 8 comments
Open
4 tasks done
Labels
bug Something isn't working needs:triage Issues that have not been investigated yet. scope:browser Related to MSW running in a browser

Comments

@aaronadamsCA
Copy link

Prerequisites

Environment check

  • I'm using the latest msw version
  • I'm using Node.js version 18 or higher

Browsers

No response

Reproduction repository

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

Reproduction steps

npm dev

Current behavior

When graphql.query infers its type arguments from a TypedDocumentNode argument, literal types in response bodies are widened by TypeScript, causing an error.

export const handler = graphql.query(MyQuery, () =>
  HttpResponse.json({ data: { parent: { foo: "BAR" } } }),
  // Type 'string' is not assignable to type '"BAR"'.
);

If you pass explicit type arguments, there is no longer an error, even though the explicit type arguments appear to be identical to the inferred type arguments.

export const handler = graphql.query<MyResult, MyVariables>(MyQuery, () =>
  HttpResponse.json({ data: { parent: { foo: "BAR" } } }),
  // No error
);

You can also work around this using const assertions, which is likely the best workaround for now.

export const handler = graphql.query(MyQuery, () =>
  HttpResponse.json({ data: { parent: { foo: "BAR" } } } as const),
  // No error
);

Expected behavior

I would expect TypeScript to evaluate identical function calls identically, regardless of whether the type arguments are passed explicitly or by inference.

Honestly, this feels more like a TypeScript issue than an MSW issue; still, maybe there's a way to resolve it at the library level, even if it just ends up being a documentation thing.

@aaronadamsCA aaronadamsCA added bug Something isn't working needs:triage Issues that have not been investigated yet. scope:browser Related to MSW running in a browser labels Feb 7, 2024
@aaronadamsCA
Copy link
Author

aaronadamsCA commented Feb 7, 2024

I opened this to follow from #1881, which I still see as more or less the same issue.

I can also see how it could potentially be solved by the new TypeScript 5.0 feature @Andarist described here: #1881 (comment)

Even if TypeScript is doing something weird internally with its type argument inference, a const type parameter modifier might solve the problem? 🤷

@Andarist
Copy link
Contributor

Andarist commented Feb 7, 2024

A somewhat isolated repro of the problem: TS playground.

Analyzing this further would probably take something between 30 minutes and a couple of hours and realistically I'm not sure when I could find such time to investigate this right now.

@aaronadamsCA
Copy link
Author

aaronadamsCA commented Feb 7, 2024

Well the good news is, assuming the reproduction is accurate, adding const to the json type parameter seems to do the trick!

  declare class HttpResponse extends Response {
    constructor(body?: BodyInit | null, init?: HttpResponseInit);
-   static json<BodyType extends JsonBodyType>(
+   static json<const BodyType extends JsonBodyType>(
      body?: BodyType | null,
      init?: HttpResponseInit,
    ): StrictResponse<BodyType>;
  }

@Andarist
Copy link
Contributor

Andarist commented Feb 7, 2024

Unfortunately, that's not the case because you don't always want to be overly eager when it comes to constifying things. The easiest way to show this is to add a mutable (aka regular) array to your desired concrete body type and use an inline array literal expression in .json(): TS playground

The ideal solution would make it possible for .json to "inherit" the contextual type from the outer call here and use that as the contextual constraint to inform what some of the used literals should be (a readonly vs mutable array, a string type vs a string literal type, etc)

@Andarist
Copy link
Contributor

Andarist commented Feb 7, 2024

I managed to slim down the repro of the original problem quite a bit: TS playground

@Andarist
Copy link
Contributor

I spent some time on this throughout the weekend (🤦‍♂️) and I don't see how this could be fixed right now. There is a TypeScript PR (see here) that I thought could help here but even after syncing it with main and tweaking a few things I couldn't make it work. That PR is about "return mapper" and the type that we are interested in here (one that could influence the contextual type for this property within the nested call) is in the main mapper of the inference context.

I was trying to tweak the algorithm (in various ways) to achieve what you want here, something was always broken though. Most notably a test case like this doesn't want the more specific type to be inferred:

enum Enum { A, B }

class ClassWithConvert<T> {
  constructor(val: T) { }
  convert(converter: { to: (v: T) => T; }) { }
}

function fn<T>(arg: ClassWithConvert<T>, f: () => ClassWithConvert<T>) { }
fn(new ClassWithConvert(Enum.A), () => new ClassWithConvert(Enum.A));

And this situation is similar to the one that you have. So the heuristic that would have to be used is not as simple as "is this a subtype of some outer inference candidate" as that kind of heuristic breaks the test case above. Different use cases, different needs. There is no "correct" answer as to how this should behave - both solutions are valid. An extra type annotation is just sometimes needed to push TS in the direction that you want. In this specific situation, I would probably just as const this string and be done with it.

I might continue thinking about this. However, I'm out of ideas right now. The best I can offer is an alternative API as the problem is related to nested inferences. An API like this avoid the problem as the "outer" inference gets computed before you get to computing the inner one (TS playground):

interface TypedDoc<T> {
  v: T;
}

type Result = {
  prop?: "BAR";
};

type Wrapped<T> = {
  wrapped: T;
};

declare function wrap<T>(t: T): Wrapped<T>;

declare function createStuff<T>(
  doc: TypedDoc<T>,
  resolve: () => Wrapped<T>,
): void;

declare const doc: TypedDoc<Result>;

createStuff(doc, () => wrap({ prop: "B" })); // error

declare function createStuff2<T>(doc: TypedDoc<T>): {
  resolve: (_: () => Wrapped<T>) => void;
};

createStuff2(doc).resolve(() => wrap({ prop: "BAR" })); // works OK

@pleunv
Copy link

pleunv commented Feb 14, 2024

Thanks @Andarist, very insightful! Quite reminiscent of the MSW v1 API, and also explains why string literals used to work fine there, and response shapes were actually strictly typed.

I managed to implement this in user-land by wrapping GraphQLHandler and passing a strictly typed HttpResponse.json as second argument in the resolver, which looks something like this:

type GraphQLResponseResolverWithResponseArg<
  Query extends GraphQLQuery = GraphQLQuery,
  Variables extends GraphQLVariables = GraphQLVariables
> = (
  info: ResponseResolverInfo<GraphQLResolverExtras<Variables>, null>,
  response: { json: typeof HttpResponse.json<GraphQLResponseBody<Query>> }
) => AsyncResponseResolverReturnType<GraphQLResponseBody<Query>>;

export function mockQuery<
  Query extends GraphQLQuery = GraphQLQuery,
  Variables extends GraphQLVariables = GraphQLVariables
>(
  document: TypedDocumentNode<Query, Variables>,
  resolver: GraphQLResponseResolverWithResponseArg<Query, Variables>,
  options?: RequestHandlerOptions
) {
  return new GraphQLHandler(
    OperationTypeNode.QUERY,
    document,
    '*',
    info => resolver(info, { json: HttpResponse.json }),
    options
  );
}

Usage:

server.use(
  mockQuery(MyQuery, (info, res) => res.json({ data: { parent: { foo: "BAR" } } }))
);

@aaronadamsCA
Copy link
Author

I'm happy to say this appears to be resolved with the release of #2107! I was able to remove several dozen as const assertions after upgrading to latest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs:triage Issues that have not been investigated yet. scope:browser Related to MSW running in a browser
Projects
None yet
Development

No branches or pull requests

3 participants