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

feat(HttpResponse): add empty response helper for strict empty responses #1807

Closed

Conversation

christoph-fricke
Copy link
Contributor

@christoph-fricke christoph-fricke commented Oct 28, 2023

This PR adds a helper that makes it possible to create a StrictResponse<null> response without a content header or type casting.

Motivation

We have some scenarios where handlers return responses without a body. This appears to be best enforced by typing the response body for resolver functions with null.

// Typing the body with "never" does not allow any response...
http.delete<never, never, never>('/resource', () => {
  return new HttpResponse(); // Errors
});

// Typing the body with "undefined" (or providing nothing) does allow responses with any body...
http.delete<never, never>('/resource', () => {
  return HttpResponse.json({id: 42}); // Should not be allowed since not empty
});

// Typing the body with "null" achieves what we are looking for
http.delete<never, never, null>('/resource', () => {
  return HttpResponse.json(null, { status: 204 }); // Works but also sets the content-type
  return new HttpResponse(null, {status: 204}) as StrictResponse<null>; // Works but needs casting
});

However, as you can see in the example, creating a response that satisfies the resolver either also sets a content-type or requires casting. Neither of these downsides should be required to create an empty response. Therefore, this PR adds HttpResponse.empty to work around this downsides, which also sets the default status to 204 as a little bonus.

http.delete<never, never, null>('/resource', () => {
  return HttpResponse.empty(); // Returns StrictsReponse<null> with 204
});

http.post<never, never, null>('/resource', () => {
  return HttpResponse.empty({ status: 201 }); // Returns StrictResponse<null> with 201
});

If you agree on adding this to MSW, I can create the relevant PR for updating the documentation for HttpResponse.empty.

@kettanaito
Copy link
Member

Hi, @christoph-fricke 👋 That's for raising this.

// Typing the body with "never" does not allow any response...
http.delete<never, never, never>('/resource', () => {
  return new HttpResponse(); // Errors
});

This looks more like a bug to me. This should compile. Maybe we can try looking into why HttpResponse doesn't like this instead?

I can't say I like the idea of HttpResponse.empty(). From the description of it, it's sole purpose is to satisfy TypeScript, so I suggest we attempt to fix the existing constructor to pass the never response body generic. That should be the correct approach here.

I want to be extremely cautious with the HttpResponse.* methods. For example, all of them must follow the Response construct signature, but in your proposed case, supporting a response body value contradicts the HttpResponse.empty() intention. This is the first hint we may be doing something wrong here.

Can you share what exact TypeScript error you get when using the never response body generic (your first example)?

@mattcosta7
Copy link
Contributor

http.delete<never, never, never>('/resource', () => {
  return new HttpResponse(); // Errors
});

I was briefly looking at this, and figured some sample errors help

Argument of type '() => HttpResponse' is not assignable to parameter of type 'ResponseResolver<HttpRequestResolverExtras<PathParams>, DefaultBodyType, never>'.
  Type 'HttpResponse' is not assignable to type 'AsyncResponseResolverReturnType<never>'

Some sample errors here:

https://www.typescriptlang.org/play?#code/JYWwDg9gTgLgBAbwBYxmANHAEqsAlAUwGdIA7IgzAZRimAGMZCSJzK4AFAQxiW6i4gimACIEAZlwCuAGxgAhCABMAngBUVYdgEEiK0vWZkKzCDIBuBKIRhSopDVoC+ccVAgg4AciEB3LwBQAShoAHRKBDIEMAQAPKQEllCYCUkpiVYAfAAUXgD0UMQQdvQEXpjZAJRwALyZiAFwcIW29nAJvti4RqwU2aSyMpUBTpUA3EEhYOGR0XHcvPyCwnBikrIKyuqa7KlZuQVFJWUV1XUNTS12pO0EnThoPWz9g9VcRHA0dIxPFPEZUEyI3Gk1wMyiMViCz4XAEQlEEmkckUqkcuwBOXyhSIxSgpXKcCqtXqCEazWi11u926xGMBBeMiGcHen1oDCYtN6cS4pBUQNGE2CYIiEPmPBhcJWayRm1ROxSg0xhxxxwJRPOpMuFLaHS6j05zwGjLeHy+7N+cSNMn5IKFYRFc3+aVuzoARK6ldjcfjTsSLuTWjcHvgDRRQjEAB4wKrAwVTcGOvbJF1WBWMz1HPEnQlnElkq5tYMW0IAKxxpAZQ1jQA

@christoph-fricke
Copy link
Contributor Author

christoph-fricke commented Oct 31, 2023

@mattcosta7 Thank you for providing these sample errors! I have been out of town for the past two days and didn't have my laptop on me.

Using never

If we consider "fixing" never what should it result to when used as the response body type?

Allow returning any Response, so the same as undefined? Does this really align with the semantics of never, which represents values that are never observed? See https://www.typescriptlang.org/docs/handbook/2/functions.html#never.

Allow returning only StrictResponse<never>? This would get weird. When Responses are created they accept BodyInit | null | undefined as the body. What value should a developer provide for the body when creating a StrictResponse<never>, especially when they want to add second argument for the ResponseInit?

Strict Empty Responses

@kettanaito I understand your hesitation towards adding more methods to HttpResponse and I am open to other solutions if they achieve the essence of this PR. I am searching for a solution to express that a response resolver is allowed to only return responses that have no body. Best I found is using null as the response body type, which results in StrictResponse<null> as the response resolver return type. All other types either result in any Response, e.g. undefined, no allowed response at all, see never in the sample errors, or other non-empty strict responses when some other type is provided. Examples of these can be found in my initial PR description.

Therefore, null and thus StrictResponse<null> look like the best option. However, I found that there is no ergonomic way of creating instances of StrictResponse<null> without including unintended headers in the response:

// This works and is ergonomic buts also sets the content-type header
const r1: StrictResponse<null> = HttpResponse.json();

// This works but isn't ergonomic
const r2: StrictResponse<null> = new HttpResponse() as StrictResponse<null>;

// This would work and is ergonomic
const r3: StrictResponse<null> = HttpResponse.empty();

Alternatively, I could imagine having HttpResponse implement StrictResponse to make the intent of this PR possible without type casting:

// Would be fine for me as well when it works without errors
const r1: StrictResponse<null> = new HttpResponse();

// This should error with this approach since a string is no empty response body
const r2: StrictResponse<null> = new HttpResponse("No Content");

@mattcosta7
Copy link
Contributor

@mattcosta7 Thank you for providing these sample errors! I have been out of town for the past two days and didn't have my laptop on me.

Using never

If we consider "fixing" never what should it result to when used as the response body type?

Allow returning any Response, so the same as undefined? Does this really align with the semantics of never, which represents values that are never observed? See https://www.typescriptlang.org/docs/handbook/2/functions.html#never.

Allow returning only StrictResponse<never>? This would get weird. When Responses are created they accept BodyInit | null | undefined as the body. What value should a developer provide for the body when creating a StrictResponse<never>, especially when they want to add second argument for the ResponseInit?

Strict Empty Responses

@kettanaito I understand your hesitation towards adding more methods to HttpResponse and I am open to other solutions if they achieve the essence of this PR. I am searching for a solution to express that a response resolver is allowed to only return responses that have no body. Best I found is using null as the response body type, which results in StrictResponse<null> as the response resolver return type. All other types either result in any Response, e.g. undefined, no allowed response at all, see never in the sample errors, or other non-empty strict responses when some other type is provided. Examples of these can be found in my initial PR description.

Therefore, null and thus StrictResponse<null> look like the best option. However, I found that there is no ergonomic way of creating instances of StrictResponse<null> without including unintended headers in the response:

// This works and is ergonomic buts also sets the content-type header
const r1: StrictResponse<null> = HttpResponse.json();

// This works but isn't ergonomic
const r2: StrictResponse<null> = new HttpResponse() as StrictResponse<null>;

// This would work and is ergonomic
const r3: StrictResponse<null> = HttpResponse.empty();

Alternatively, I could imagine having HttpResponse implement StrictResponse to make the intent of this PR possible without type casting:

// Would be fine for me as well when it works without errors
const r1: StrictResponse<null> = new HttpResponse();

// This should error with this approach since a string is no empty response body
const r2: StrictResponse<null> = new HttpResponse("No Content");

I think never is probably incorrect as well

FWIW undefined seems to work where null and never don't

http.delete<PathParams, DefaultBodyType, undefined>('/resource', () => {
  return new HttpResponse()
});

this appears related to this type:

export type ResponseResolverReturnType<
BodyType extends DefaultBodyType = undefined,
> =
| (BodyType extends undefined ? Response : StrictResponse<BodyType>)
| undefined
| void

Which could be extended to account for null as well as undefined, but probably not never?

@mattcosta7
Copy link
Contributor

http.delete<PathParams, DefaultBodyType, undefined>('/resource', () => {
  return new HttpResponse()
});
http.delete<PathParams, DefaultBodyType, undefined>('/resource', () => {
  return new HttpResponse(JSON.stringify({key: 'value'}), {headers: {'Content-Type': 'application/json'}})
});
http.delete<PathParams, DefaultBodyType, undefined>('/resource', () => {
  return new HttpResponse(null, {})
});

each of these works fine as an empty response, since they'll accept a valid Response object as a return which HttpResonse extends

@christoph-fricke
Copy link
Contributor Author

I think we have two separate topics here. First one being never as response body type and the second topic is about strictly-typed empty responses (null).

Regarding never: I don't think there is much we can do without some weirdness in other places. It might be changed to behave as undefined, but they have separate meanings so combining them in the same behavior seems weird.


I am way more interested in strictly-typed empty responses, which this PR is all about:

null is working perfectly fine. I want it to result in StrictResponse<null> as the resolver function's return type. Combining its behavior with undefined, i.e. resulting in Response would break my use case.

I am approaching this topic from the angle of Openapi-MSW. I want to express that a handler can only return responses with empty content as StrictResponse<null> expresses, and not any content as Response expresses.

OpenAPI-MSW is all about surfacing type conflicts in MSW resolvers when they no longer match their OpenAPI specification, e.g. return a response body that does not match the defined response schema. Imagine an API endpoint that returns some response content and is now changed to return no-content with the response. This should surface a type error in MSW handlers for this endpoint since they would still return a response and have to be adjusted.

And indeed, this scenario does surface a type error when the response body generic of the MSW resolver function gets typed with null (=> StrictResponse<null>), which OpenAPI-MSW is doing since I merged this PR: christoph-fricke/openapi-msw#17. This works well and is good. 👍

However, I don't thing the current way of creating strictly typed responses with no content (StrictResponse<null>) provides a good DX, which is what I am trying to change with this PR. The code examples from my previous comments to showcase how I think the DX could be improved.

I hope that the OpenAPI scenario better explains what I am trying to achieve and why. I created a small Playground that hopefully helps explaining it even more: Link to Playground

@kettanaito
Copy link
Member

kettanaito commented Nov 1, 2023

Thanks for the detailed discussion, @christoph-fricke, @mattcosta7! Will provide my input below.

On default body types

The DefaultBodyType type that both request and response bodies extend already allows both undefined and null as accepted values:

export type DefaultBodyType =
| Record<string, any>
| DefaultRequestMultipartBody
| string
| number
| boolean
| null
| undefined

The difference is like what you'd expect:

  • undefined, the response has no body;
  • null, the response has an empty body.

JavaScript is a bit not in our favor here because runtime has no distinction between null and undefined response body init, although there is a strong semantic distinction. But this is what runtime JavaScript results in:

new Response().body
// null

new Response(undefined).body
// null

new Response(null).body
// null

I'd argue that the first example must return undefined as the body value of the response because that's precisely what was given to the Response constructor.

On new HttpResponse() strict body type

Right now, the HttpResponse class directly extends the global Response class, which has no strict body type. I believe this is why the proposal is here. And this is also the main difference and the reason why HttpResponse.text() and HttpResponse.json() can have strict response body type—because they return a ScriptResponse custom type that achieves strict response body type through the symbol-based type.

What we can try to do here is to make HttpResponse implement the StrictResponse type. It will still be a type-only kind of response body validation but that's precisely what we're after. We can also turn the StrictResponse into a branded type instead of introducing an internal bodyType symbol, I think that's redundant.

But that likely won't work. TypeScript is notoriously bad with null/undefined distinction. Here's an example (in this example, the validate(new Foo(null)) call must type error because Foo<null> doesn't extend the required type of Foo<undefined> in theory; in practice, it does).

I don't think that purely type-level safety is possible in this regard. I don't think that introducing HttpResponse.empty() is a good solution for this either. We are after type-safety, and as such, as must implement the solution on the type level.

This is also a good moment to mention that request/response body type safety is purely imaginary. In reality, nothing can guarantee what body the application will receive, so, technically speaking, bodies must always be unknown and then explicitly narrowed both type- and runtime-wise to a particular type. This reminds me of the sentiment that @pcattori shared with me on proper type safety and how type-casting is beneficial at times. MSW may be trying to do you a favor with the StrictRequest/StrictResponse but, as a result of it, shooting itself in the foot.

@kettanaito
Copy link
Member

Actually, my bad, I'm always forgetting about this TS hack to know if a generic was provided:

type X<T> = [T] extends [never] ? 'NOT PROVIDED' : 'PROVIDED'

Alas, we cannot use this for StrictResponse because:

A class can only implement an object type or intersection of object types with statically known members.ts(2422)

🫠

@christoph-fricke
Copy link
Contributor Author

@kettanaito Thank you for your input! I will have a look at this again this weekend and convert the PR from HttpResponse.empty() to implementing a branded type or StrictResponse for HttpResponse directly.

This is also a good moment to mention that request/response body type safety is purely imaginary. In reality, nothing can guarantee what body the application will receive, so, technically speaking, bodies must always be unknown and then explicitly narrowed both type- and runtime-wise to a particular type.

I think this is only partially true, right? You are right that in a client/server setting, with both specifying the same request- and response-body for a communication, they can't rely on the incoming data matching the specified type. Meaning, the client can't rely on it's response body type and the server cannot rely on the request body type.

However, for the client the request body is a contract that must be fulfilled by its requests, and for the server the response body type is a contract that must be fulfilled by its response. They can make sure that the type of the data they own is matched. When both rely on the same contract types, they can make sure that they at least fulfill their own bargain to make the communication work.

Client:                            Network   Server:
  - Request Type: Contract         <----->     - Request Type: Potential Lie
  - Response Type: Potential Lie   <----->     - Response Type: Contract

Reflecting this back onto MSW handlers: The request body type is kinda a lie that we have for convenience. But the response body type is able to act as a contract that resolvers have to fulfill to ensure that they return a response that is expected by the application (client). Of course, this doesn't matter if the real backend returns completely different responses, but this is where OpenAPI comes into play to specify a communication contract. ;)

@kettanaito
Copy link
Member

kettanaito commented Mar 30, 2024

The recent changes about a proper response body type validation can help us implement this (#2107). In fact, I can achieve this behavior that validates the empty (null) response body correctly:

Screenshot 2024-03-30 at 10 37 46

My only comment would be that it'd be nice if that type error happened on the argument to new HttpResponse() vs on the return type of the response resolver. It does happen in the correct place for HttpResponse.text() and HttpResponse.json() methods, so not sure why the constructor is different. The only thing that differs it is that the constructor cannot have generic type arguments so I'm adding the argument to the class itself.

Edit: I think I know the difference that causes it. HttpResponse.text() returns a StrictResponse<T> type while new HttpResponse() always returns the HttpResponse<T> type (even if it implements StrictResponse<T>). In other words, the return type of the constructor is not strictly compatible with the return type expected by the resolver (StrictResponse). I think that's why TypeScript lifts that error as a return type error, not the constructor argument error.

@kettanaito
Copy link
Member

I've opened a pull request that will add support for explicit empty response bodies by using null as the response body type generic argument:

https://github.com/mswjs/msw/pull/2118/files#diff-cb924248fca260b733cd71049d2b17c14cf5c52ef561273ab8e420b1d5a674a5R88-R104

@christoph-fricke, could you please take a look at that pull request and confirm that it satisfies your use cases? Thanks.

@kettanaito
Copy link
Member

I will close this in favor of #2118. It achieves the original use case without having to resort to custom response static methods. Thanks for raising this, @christoph-fricke! I can still use your wise eyes on that pull request.

@kettanaito kettanaito closed this Apr 2, 2024
@christoph-fricke christoph-fricke deleted the feat/empty-response branch August 7, 2024 18:03
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.

3 participants