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

fix(core): export the "RequestHandlerOptions" type #1774

Merged

Conversation

christoph-fricke
Copy link
Contributor

What has changed`

This PR adds additional exports for the HttpRequestResolverExtras and RequestHandlerOptions to the main package export.

Why has this changed?

I am currently in the process of writing a small wrapper around MSW's http methods that is supposed to use type definitions generated from an OpenAPI spec with openapi-ts. The goal is to be able to have conflicts in http-handler definitions when they are no longer compatible with the OpenAPI spec. Here is some pseudocode that hopefully provides an idea about my intention with "wrapping msw":

import {
  http as http,
  type HttpHandler,
  type ResponseResolver,
  type DefaultBodyType,
} from "msw"; 

function createOpenApiHttp<ApiDef>() {
  return {
    get: <Path extends PathsFromApiDef>(
      path: Path,
      resolver: ResponseResolver<
        PathParamsFromApiDefForPath<Path>,
        RequestBodyFromApiDefForPath<Path>,
        ResponseFromApiDefForPath<Path>
      >,
      options: RequestHandlerOptions
      //        ^ Interface cannot be imported because it is not exported
    ): HttpHandler => http.get(path as string, resolver, options),
     //                                            ^ resolver cannot be
     // passed to http.get(...) because the params generic
     // is not wrapped with `HttpRequestResolverExtras`, which not exported


    // ... other methods that exist on http
  }
}

I am open for other ideas that make it possible to wrap the http methods without exporting the two additional interfaces. However, this is the only solutions that I found, unless it is somehow possible nowadays to provide generic arguments to typeof http["get"]", i.e. <never, never, never>(typeof http["get"]).

Should we rather export some new type helpers than these additional two interfaces? I open for your suggestions.

@kettanaito
Copy link
Member

Thanks for opening this, @christoph-fricke! I think this is the right move.

I wonder if we can expose a more developer-friendly type to make annotations of custom handlers/resolvers a bit better. How would you imagine that type to work? 🤔

@christoph-fricke
Copy link
Contributor Author

@kettanaito I am not quite sure. I can only think of exporting the type of the http methods, so basically:

export type HttpMethod<
  Params extends PathParams<keyof Params> = PathParams,
  RequestBodyType extends DefaultBodyType = DefaultBodyType,
  ResponseBodyType extends DefaultBodyType = undefined,
> = (
  path: Path,
  resolver: ResponseResolver<
    HttpRequestResolverExtras<Params>,
    RequestBodyType,
    ResponseBodyType
  >,
  options?: RequestHandlerOptions,
) => HttpHandler;

However, for my use-case I would still have to destruct the resolver and options Parameters, because the path is typed stronger based on the OpenApi schema. At least, it is then possible to do provide generics when deconstructing the type:

type Resolver = Parameters<HttpMethod<MyParams, MyReqBody, MyResBody>>[1]

Alternatively, I think the only option is to make typing the ResponseResolver less verbose by having it wrap the Params internally with HttpRequestResolverExtras.

@christoph-fricke
Copy link
Contributor Author

christoph-fricke commented Oct 17, 2023

Update: I temporarily defined the HttpMethod from my previous comment locally to see if it helps. De-structuring the various parameters from that is relatively clean as they can be assigned to helper types. Since the helper can also receive further generics, this remains quite flexible for each specific use case.

I am just not to sure about the name HttpMethod suggests something else but HttpHandlerFactory also sound a bit dull.

@kettanaito
Copy link
Member

kettanaito commented Oct 18, 2023

@christoph-fricke, did you try using the generics of http.get<Params, RequestBody, ResponseBody> to implement this custom function?

The thing is, your abstraction seems to be entire type-focused. As in, you don't provide any extra functionality to http.* handlers. As such, you should implement that abstraction in types alone. Here's an example:

interface OpenApiHandlers<ApiDefinition> {
  get: typeof http.get<
    PathParamsFromApiDefForPath<ApiDefinition>,
    RequestBodyFromApiDefForPath<ApiDefinition>,
    ResponseFromApiDefForPath<ApiDefinition>
  >
  // ...the rest of "http.*" handlers.
}

export const openApi = http satisfies OpenApiHandlers<Def>
openApi.get(path, resolver)

One thing that MSW doesn't support right now is the Path generic, which I assume you want to be extracted from the ApiDefinition and be more strict, is that correct?

I recall Path used to be a generic but a decision was made to remove it. Let me reflect on why that was necessary.

Or even better, this is how you can customize the path:

type AnyFunction = (...args: any) => any
type SkipFirst<List extends Array<unknown>> = List extends [
  infer _First,
  ...infer Rest,
]
  ? Rest
  : never

type HttpHandlerWithPath<
  ApiDefinition,
  P extends Path = PathsFromApiDef<ApiDefinition>,
  Params extends PathParams = PathParamsFromApiDefForPath<P>,
  RequestBody extends DefaultBodyType = RequestBodyFromApiDefForPath<P>,
  ResponseBody extends DefaultBodyType = ResponseFromApiDefForPath<P>,
  HandlerType extends AnyFunction = typeof http.get<
    Params,
    RequestBody,
    ResponseBody
  >,
> = (path: P, ...args: SkipFirst<Parameters<HandlerType>>) => HandlerType

interface OpenApiHandlers<ApiDefinition> {
  get: HttpHandlerWithPath<ApiDefinition>
}

function createOpenApi(): OpenApiHandlers<{}> {
  return http
}

@kettanaito
Copy link
Member

On the Path generic

If I recall it right, we used to have the Path generic in the rest.* handlers but it was removed. There were two main reasons behind that change:

  1. Practical. As a developer, when you want to customize, say, a path parameter or a request body, having to also provide a general string | RegExp as the first generic is tiresome.
// Why should I provide a generic "string" if all I want
// is to customize the request body type?
http.post<string, RequestBody>('/resource', resolver)

I believe then we moved the Path generic to be the last in the generics list for the handlers. That's not really the solution to the problem.

  1. Path inference. We experimented in the past with path inference where the path type would be inferred from the path literal value, including path parameters.
// The "Path" generic would infer from "/user/:id",
// and infer its parameter "id" to annotate the 
// "params" key in the resolver argument. Neat!
http.get('/user/:id', resolver)

That was nice until the developer wanted to customize the request/response body types. Now, the inference would break because there isn't partial inference in TypeScript (at least it used to be so):

http.get<???, RequestBody>('/user/:id', resolver)

// This stays true even if "Path" is the last generic!
// The entire path below will be simply a "string".
http.get<Params, RequestBody>('/user/:id', resolver)

With that in mind, looks like we decided to ditch the path generic altogether.

The present

There may be some work done about partial inference in TypeScript with the recent releases so we may eventually be able to support that feature. I still think having Path in the generics to http.* handlers can be a bit of a nuisance for people who don't want to have strict path-based API mocking and would still have to provide the generic value if they want to modify params/request/response body generics.

@christoph-fricke
Copy link
Contributor Author

christoph-fricke commented Oct 18, 2023

@kettanaito Ohhhhh! I tried all sorts of ways to provide generics totypeof http["get"], which did not work, but I never not tried typeof http.get<...>. Thank you so much! This little hint has made my whole day. 🚀

This makes the proposed type helper HttpMethod totally worthless since it is basically just an alias. Do we event want to export RequestHandlerOptions then, since the can also be de-structured with Parameters<typeof http.get<...>>? Otherwise, we can also close this PR.

Motivation for my openapi-msw wrapper

You are right that it is mostly just wrapping http at the type level. One of my current goals is to layer as little as necessary on top of MSW to remain versatile. Since MSW is already able to provide type safety for the path params and request/response body, it is mostly a matter of extracting these types from definitions generated by openapi-ts.

As you wrote, the biggest goal is to also provide type safety for the path so changes in the spec result in conflicts in the handler definitions. Therefore, it needs a wrapper at the type level as you correctly identified, but also a small runtime wrapper to map paths from the OpenAPI spec.

In the OpenAPI spec, path templates1 are delimited by curly braces, e.g. /users/{id}. This path syntax is also used by openapi-ts in the string literals for the generated paths. However as you know, MSW and most of the Node.js ecosystem uses colon templates, e.g./users/:id.

Therefore, the wrapper could either map the string literals at the type level, or stay true to the OpenAPI spec and accept paths with curly braces as arguments, which are converted at runtime before forwarding them to MSW. Currently, I have decided for the small runtime wrapper and embrace the OpenAPI spec - basically being OpenAPI first.

If you are interested in this, you can find my work-in-progress solution here.

Openapi-ts also generates information about response body provided for each return status. In the future, I might consider a beefed-up version of HttpResponse that has additional typings based on this information, but for now I want to keep the API close to MSW.

Footnotes

  1. https://spec.openapis.org/oas/v3.0.3#path-templating

@kettanaito
Copy link
Member

Therefore, the wrapper could either map the string literals at the type level, or stay true to the OpenAPI spec and accept paths with curly braces as arguments, which are converted at runtime before forwarding them to MSW.

100% recommend covering this on the type level. You can use string literal types to cast any expression to any other expression, and this will convert your /foo/{bar} string types to /foo/:bar with ease. Don't pay with runtime for this.

You're doing great job on this! Keep it up. I have an OpenAPI -> request handlers generator as internal tooling and perhaps you would be interested in joining efforts and polishing it together? If you are, let me know.

While HttpRequestResolverExtras is an internal type, I think we should export the options type. I will adjust the pull request.

@kettanaito kettanaito changed the title feat(core): export HttpRequestResolverExtras and RequestHandlerOptions types feat(core): export the "RequestHandlerOptions" type Oct 20, 2023
@kettanaito kettanaito changed the title feat(core): export the "RequestHandlerOptions" type fix(core): export the "RequestHandlerOptions" type Oct 20, 2023
@kettanaito kettanaito merged commit 7d6d82d into mswjs:feat/standard-api Oct 20, 2023
8 checks passed
@kettanaito
Copy link
Member

Welcome to contributors, @christoph-fricke! 👏

@christoph-fricke christoph-fricke deleted the feat/expose-resolver-types branch October 28, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants