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

Narrowing the response body type in HttpResponse.json #2105

Closed
1 task
Hajime-san opened this issue Mar 22, 2024 · 16 comments · Fixed by #2107
Closed
1 task

Narrowing the response body type in HttpResponse.json #2105

Hajime-san opened this issue Mar 22, 2024 · 16 comments · Fixed by #2107

Comments

@Hajime-san
Copy link

Hajime-san commented Mar 22, 2024

Scope

Improves an existing behavior

Compatibility

  • This is a breaking change

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
Copy link
Member

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
Copy link
Member

kettanaito commented Mar 22, 2024

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:

static json<BodyType extends JsonBodyType>(
body?: BodyType | null,
init?: HttpResponseInit,
): StrictResponse<BodyType> {

@kettanaito kettanaito changed the title Narrowing type HttpResponse.json Narrowing the response body type in HttpResponse.json Mar 22, 2024
@kettanaito
Copy link
Member

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

@kettanaito kettanaito added the help wanted Extra attention is needed label Mar 22, 2024
@Andarist
Copy link
Contributor

@kettanaito
Copy link
Member

kettanaito commented Mar 22, 2024

@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
Copy link
Contributor

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
Copy link
Member

kettanaito commented Mar 22, 2024

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

But this would still work, would it not?

Yes.

@kettanaito kettanaito reopened this Mar 22, 2024
@kettanaito
Copy link
Member

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

@Andarist
Copy link
Contributor

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

@kettanaito
Copy link
Member

I'm implementing this in #2107. I think this is a good direction to follow.

@kettanaito kettanaito self-assigned this Mar 25, 2024
@kettanaito kettanaito removed the help wanted Extra attention is needed label Mar 25, 2024
@Hajime-san
Copy link
Author

Thank you for implementing this feature!

@kettanaito
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants