christoph-fricke/openapi-msw

Issue: Can’t infer response values other than 200

Closed this issue · 8 comments

margalit commented

Hi there! Great library, I’m trying to integrate it and having an issue mocking responses in that it only seems to validate a 200 response not any other codes eg.

import { HttpResponse } from 'msw'
import { createOpenApiHttp } from 'openapi-msw'

interface paths {
  '/': {
    /** Root */
    get: {
      responses: {
        /** @description Successful Response */
        200: {
          content: {
            'application/json': {
              ok: true
            }
          }
        }
        /** @description Validation Error */
        422: {
          content: {
            'application/json': {
              ok: false
            }
          }
        }
      }
    }
  }
}

const http = createOpenApiHttp<paths>()

/** Works */
http.get('/', () => {
  return HttpResponse.json({ ok: true as const }, { status: 200 })
})

/** Errors */
http.get('/', () => {
  return HttpResponse.json({ ok: false as const }, { status: 422 })
})
christoph-fricke commented

Hi @margalit! Thank you for your feedback. The typing of response bodies, especially error cases, isn't fully solved yet. Right now, the response body typing is getting extracted the same way openapi-fetch does it, which picks the first "2XX" response. As you have noticed, this makes it hard to return error responses, let alone unknown status codes.

It is something that I want to tackle but I haven't found a good solution yet. There are some things to consider that make it quite hard to solve well:

  • Many APIs do not have a proper specification for all possible error statuses for every endpoint. Especially, common ones like 401 or 500 are often not specified. Under this circumstance, I think it probably a good idea to be able to return any error response for unspecified statuses.
  • A specified error status should only be allowed to return the specified response content. Same obviously is required for successful responses as we have right now. So strict typings for a known statuses to have the benefits of type errors when the OpenAPI spec changes.
  • What about response content types. Not really common, but the same status code can return different responses, e.g. text or JSON, depending on the content type. Both text and JSON can be strictly typed with MSW's StrictResponse. Thinks get a bit more funky and have to be inspected more closely for other media types.

Typing the response body in MSW is rather restricted. We can force it to either be Response, which is any response and looses the benefit of TS errors on incompatible OpenAPI spec changes. Alternatively, we can use StrictResponse<BodyType> for strict typing of the body. To achieve the idea of "allowing responses for any unknown error status" we could use the union Response | StrictResponse<BodyType>. However, this allows returning any response again, thus not erroring when the happy path does not match.

To work around this, I have been thinking about providing my own HttpResponse to resolvers, which can contain more type inference based on the endpoint. I am still trying to figure out a good API for this and the scope. How can it look and feel nice while providing great type inference? How far do we want to push strict typing for response content types? Based on your example, it could look like this (mixed in a few API variations):

import { createOpenApiHttp } from 'openapi-msw'

interface paths {
  '/': {
    /** Root */
    get: {
      responses: {
        /** @description Successful Response */
        200: {
          content: {
            'application/json': {
              ok: true
            }
          }
        }
        /** @description Validation Error */
        422: {
          content: {
            'application/json': {
              ok: false
            }
          }
        }
      }
    }
  }
}

const http = createOpenApiHttp<paths>()

http.get('/', ({ HttpResponse }) => {
  // For great type inference while defining a response, I think the order has to be:
  // - status
  // - media type
  // - body
  return HttpResponse.status(200).json({ ok: true as const }, { /* response init */ });
})

http.get('/', ({ HttpResponse }) => {
  // Maybe only suggest methods like ".json()" and ".text()" depending on the media type
  // Errors, because .text() doesn't exist for the status code
  return HttpResponse(200).text("Something"); // JS conventions would suggest `httpResponse(200).text("Something")`
})

http.get('/', ({ HttpResponse, request }) => {
  if(!authenticated(request)) {
    // Unknown status code
    return HttpResponse(401, "json", { body: "content" })
  }
  // Errors, since status is known and {ok: true} is not assignable to {ok: false}.
  return HttpResponse.json({ ok: true as const }, {status: 422})
  // Problem: I think TS cannot make suggestions for the body before the status is added as the second arg...
})

Under the hood, the resolver return type might still be typed with the first successful response body type to avoid breaking changes and make the API opt-in. The custom HttpResponse would provide strict typings for its arguments, which also makes TS error messages around the response body easier to read, but just returns a typing that matches the resolver return type.

margalit commented

Thanks for the reply @christoph-fricke! Makes a lot of sense.

It looks like openapi-fetch also has a ErrorResponse type that plucks out the first error response, similar to the SuccessResponse type. I wonder if in the short term we could rework the type of the response as a union of either the ErrorResponse or SuccessResponse? Similar to the FetchResponse here?

I know this isn't as detailed a solution as the one you outlined but would unblock my use case.

christoph-fricke commented

@margalit I would like to avoid releasing a short term solution as a "workaround" without considering all the implications of it. Having to remove the solution again would need a breaking change.

I just released #21 so I can start to fully focus on better response typing, which I will hopefully get out of the door soon. Would it unblock you for now, to "just type cast" error response content to successful response content?

margalit commented

@christoph-fricke no problem, thanks for the thoughtful response! Would you like me to close this issue or is it okay to be kept open?

christoph-fricke commented

Let's keep it open! After all, it is a downside that affecting the usability of the library. Hopefully, I have it addressed soon.

christoph-fricke commented

@margalit I started to chip away on a response helper implementation that looks quite promising. It types responses bodies based on the status and media type, which solves the current limitations around error responses. Furthermore, I love the improved TS experienced of having response-body errors appear for the actual response body and not as super long errors for the resolver function. Please let me know what you think. :)

API Definition: https://github.com/christoph-fricke/openapi-msw/blob/feat/response-helper/src/response.ts#L9
Usage example: https://github.com/christoph-fricke/openapi-msw/blob/feat/response-helper/test/response-content.test.ts#L46

christoph-fricke commented

@margalit I noticed that the comments on the helpers from openapi-typescript-helpers kinda lie and we already return a union for all successful status code responses. This causes a problem when they have different media types.

I created a fix for this and incorporated your suggestion of have having a union of response types for all status codes while I am at it. However, this also has one downside in the experience of writing MSW handlers, which I described in #42. I would love to get your opinion on this as I feel like this PR solves existing problems, but solving it makes the DX slightly worse.