mswjs / msw

Industry standard API mocking for JavaScript.
https://mswjs.io
MIT License
15.98k stars 520 forks source link

Narrowing the response body type in `HttpResponse.json` #2105

Closed Hajime-san closed 8 months ago

Hajime-san commented 8 months ago

Scope

Improves an existing behavior

Compatibility

Reproduction steps

Versions:

"msw": "2.2.10"

Feature description

Following the documentation of Using with TypeScript, it works almost well. But, it allows widing type of HttpResponse.json.

Here is an modified Higher-order request handlers example of adding non-existence foo field to HttpResponse.json.


import { http, HttpResponse, HttpResponseResolver } from 'msw';

type SdkRequest = {
  transactionId: string
}

type SdkResponse = {
  transactionId: string
  data: { ok: boolean }
}

function handleSdkRequest(
  resolver: HttpResponseResolver<never, SdkRequest, SdkResponse>
) {
  return http.post('https://some-sdk.com/internal/request', resolver)
}

export const handlers = [
  handleSdkRequest(async ({ request }) => {
    const data = await request.json()
    return HttpResponse.json({
      transactionId: data.transactionId,
      // `foo` should be error
      data: { ok: true, foo: 'bar' },
    })
  }),
]
kettanaito commented 8 months ago

Hi, @Hajime-san. Thanks for this suggestion, I think it makes perfect sense.

A bit of context why it allows additional fields right now. Since the response body type is provided onto the handler itself, there's no way to "drill" it to the argument type of HttpResponse.json(). Instead, what happens is this:

export const handlers = [
  handleSdkRequest(async ({ request }) => {
    const data = await request.json()
    // Body type is inferred from the argument you provide
    // to the HttpResponse.json().
    return HttpResponse.json({
      transactionId: data.transactionId,
      // `bar` should be error
      data: { ok: true, foo: 'bar' },
    })
  }),
]

The inferred body type becomes this:

HttpResponse.json<{ transactionId, data: { ok, foo } }>

This type is later compared to the return type of the entire resolver, which is inferred from your response body type argument (the narrow type). It seems, since it matches partially, it passes the validation.

Reproduction

Here's this issue distilled to a minimal reproduction scenario outside of MSW:

https://www.typescriptlang.org/play?#code/CYUwxgNghgTiAEYD2A7AzgF3gIycAngCr4AOIAXPAK4oCWAjlQmvgLa4QBQnIAHiUhhZaKDCBgAzKGAQBlDDFpgMAJRBoB6EAB4AQniKkQAPk7x4fMSmBp4ajajQIA3mfhwowVBHzwA2rgExGQAupT6QUacAL7cGEZ26ppOEYZk8AC88M7wEkhIlLhIECBQKPCxnBI0yrSo8AAWZcAlMAAUAJSU8orK9sk6-Y4gqcEm2W5wGFQw5QBWaKhtOXkF8ApMADQ4sJQA5Lz4AF57FR0x3NUotfULqNqExm2B+JSEXfA9SqpJww-GE3MUxm5Wc0XgUFsZXwFyAA

kettanaito commented 8 months ago

Note that if you provide the narrow response body type to HttpResponse.json<*>(), you will get the behavior you want:

return HttpResponse.json<SdkResponse>({ ... })

Playground showcasing the "unknown property 'bar'" type error.

Since HttpResponse is imported separately from the response resolver, I don't believe there's a way for the resolver to "forward" that response body type onto the HttpResponse.json() call. The body argument type will always be inferred, and if it matches with extra properties, that will satisfy the type in TypeScript's eyes:

https://github.com/mswjs/msw/blob/16fadfeb872c156cc610977cc5507630d59146ad/src/core/HttpResponse.ts#L79-L82

kettanaito commented 8 months ago

@Andarist, please, correct me if I'm wrong, Mateusz.

Andarist commented 8 months ago

https://twitter.com/TitianCernicova/status/1770789543509168365

kettanaito commented 8 months ago

@Andarist, but this case is a bit different, isn't it? I can clearly get a proper narrow type if I provide an explicit body generic. The problem is not how the response body type is declared but how to project that type onto the HttpResponse.json() function's argument.

// Works if the body type is explicitly provided.
// Hijacks inference from the argument value.
+return json<ResponseBodyType>({ foo: true, bar: 'xyz' })
// Breaks because now the T in json<T> gets inferred from
// a broader value and it satisfies the type. 
-return json({ foo: true, bar: 'xyz' })

I get your point that a broader type satisfying a narrower type is how types work. I was mostly curious if we can make this type projection possible? Because in this case it does feel like a bug to the user.

Andarist commented 8 months ago

This is likely very close to the stuff I was investigating here. You can't easily influence how TS infers such things.

You could use NoInfer to get the desired behavior here (TS playground) but you don't really want this because that will prevent json from "working on its own".

The essence here is likely something around those lines: the contextual return type is a lower priority inference and an inference made directly from the arguments will always "win" over it.

kettanaito commented 8 months ago

You could use NoInfer to get the desired behavior

TIL NoInfer. This makes me consider if HttpResponse.json() has a use on its own. I'm tempted to say it doesn't. You can certainly use it to construct a Response anywhere you wish but it's designed to work with response resolvers (hence the magic with the strict body type) so it may be reasonable to get StrictResponse<unknown> unless you (a) put it in a typed resolver; (b) provide the body type explicitly as json<T>.

Are there any other implications to using NoInfer?

kettanaito commented 8 months ago

Alas, NoInfer has been introduced only in v5.4. Sigh.

I don't support one can check if a built-in type exists before using it.

@Hajime-san, it looks like there's no capabilities in TypeScript to achieve what you want (see a fantastic explanation). You have one option: provide an explicit response body type to HttpResponse.json<HERE>().

Andarist commented 8 months ago

Are there any other implications to using NoInfer?

you'd break the simplest things like

const j = json({ foo: 'bar'})
return j

So it could lead to surprising DX.

Alas, NoInfer has been introduced only in v5.4. Sigh.

Right. You can mimic it using this though:

type NoInfer<T> = [T][T extends any ? 0 : never]
kettanaito commented 8 months ago

you'd break the simplest things like

But this would still work, would it not?

const j = json<T>(value)
return j // StrictResponse<T>

I'd say it's fine to have StrictResponse<unknown> if you haven't provided an explicit generic. That even makes sense. I wonder if we can replace unknown here with DefaultBodyType since we, technically, know the constraints of the response bodies transferred over the network.

Andarist commented 8 months ago

But this would still work, would it not?

Yes.

kettanaito commented 8 months ago

@Andarist, I'm trying your suggestion but not getting the same behavior for custom NoInfer. Is something off, in your opinion?

Andarist commented 8 months ago

You only need to wrap T and nothing else: TS playground

kettanaito commented 8 months ago

I'm implementing this in https://github.com/mswjs/msw/pull/2107. I think this is a good direction to follow.

Hajime-san commented 8 months ago

Thank you for implementing this feature!

kettanaito commented 8 months ago

Released: v2.2.11 🎉

This has been released in v2.2.11!

Make sure to always update to the latest version (npm i msw@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.