msw icon indicating copy to clipboard operation
msw copied to clipboard

Type error when `graphql.query` infers type arguments from `TypedDocumentNode`

Open aaronadamsCA opened this issue 1 year ago • 9 comments

Prerequisites

Environment check

  • [X] I'm using the latest msw version
  • [X] 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 avatar Feb 07 '24 12:02 aaronadamsCA

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: https://github.com/mswjs/msw/issues/1881#issuecomment-1932067175

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

aaronadamsCA avatar Feb 07 '24 15:02 aaronadamsCA

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.

Andarist avatar Feb 07 '24 15:02 Andarist

~~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>;
  }

aaronadamsCA avatar Feb 07 '24 17:02 aaronadamsCA

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 avatar Feb 07 '24 17:02 Andarist

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

Andarist avatar Feb 07 '24 19:02 Andarist

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

Andarist avatar Feb 12 '24 11:02 Andarist

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" } } }))
);

pleunv avatar Feb 14 '24 14:02 pleunv

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.

aaronadamsCA avatar Mar 31 '24 21:03 aaronadamsCA