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

Invalid response status after update to 0.4.0 #25

Closed
gabscogna opened this issue Oct 25, 2024 · 4 comments · Fixed by #27
Closed

Invalid response status after update to 0.4.0 #25

gabscogna opened this issue Oct 25, 2024 · 4 comments · Fixed by #27
Assignees
Labels
bug Something isn't working fix available

Comments

@gabscogna
Copy link

gabscogna commented Oct 25, 2024

I ran in problem with the newer version.
I have a test like this:

// ...
fetchMocker.mockResponseOnce('', { status: 204 });
// ...

But when I run the tests, it give me the error:

TypeError: Response constructor: Invalid response status code 204
 ❯ buildResponse node_modules/vitest-fetch-mock/src/index.ts:388:12
 ❯ node_modules/vitest-fetch-mock/src/index.ts:59:1

It should accept send null as response body here:

type ResponseBody = string;

type ResponseBody = string | null;

...and here:

if (typeof responseProviderOrBody === 'string') {

if (responseProviderOrBody === null || typeof responseProviderOrBody === 'string') {

So this will work:

// ...
fetchMocker.mockResponseOnce(null, { status: 204 });
// ...
@dirkluijk
Copy link
Collaborator

dirkluijk commented Oct 25, 2024

Took me a moment to figure out what's happening here, but I found out.

Before, we were relying on cross-fetch:

import { Response } from 'cross-fetch';

new Response('', { status: 204 })

which works fine.

However, when running new Response('', { status: 204 }) in a Node.js or browser environment without polyfills, it throws an error:

  • Failed to construct 'Response': Response with null body status cannot have body in Chrome
  • TypeError: Response constructor: Invalid response status code 204 in Node.js
  • TypeError: Response with null body status cannot have body in Deno

This is by design. There are two solutions:

  1. rewrite your code to:
// either:
fetchMocker.mockResponseOnce(() => ({ status: 204 }));
// or:
fetchMocker.mockResponseOnce(() => new Response(null, { status: 204 }));
  1. we adjust vitest-fetch-mock to accept null as response body, which makes sense to me, but note that this wasn't possible in previous versions either. We just broke it by replacing the incorrect polyfill with the correct implementation.

@IanVS let me know what you think. I'm willing to make a fix if we agree on solution 2.

@dirkluijk
Copy link
Collaborator

dirkluijk commented Oct 26, 2024

Aside from making the response body nullable in mockResponse(string: ResponseBody, params?: MockParams), I opened another MR which enables you to also write:

fetchMocker.mockResponseOnce({ status: 204 });
fetchMocker.mockResponseOnce(new Response(null, { status: 204 }));

To allow passing null as response body, I opened a separate MR.

@dirkluijk dirkluijk added bug Something isn't working fix available labels Oct 28, 2024
@dirkluijk dirkluijk linked a pull request Oct 28, 2024 that will close this issue
IanVS pushed a commit that referenced this issue Oct 28, 2024
This PR makes it possible to use more types of overloaded functions:

```ts
fetch.mockResponseOnce('x');
fetch.mockResponseOnce({ body: 'x' }); // previously not allowed
fetch.mockResponseOnce(new Response('x'));  // previously not allowed

fetch.mockResponseOnce(() => 'x');
fetch.mockResponseOnce(() => ({ body: 'x' }));
fetch.mockResponseOnce(() => new Response('x'));

fetch.mockResponseOnce(() => Promise.resolve('x'));
fetch.mockResponseOnce(() => Promise.resolve({ body: 'x' }));
fetch.mockResponseOnce(() => Promise.resolve(new Response('x')));
```

This could make certain cases less awkward, like
#25 (comment):
```ts
fetch.mockResponseOnce({ status: 204 });
fetch.mockResponseOnce(new Response(null, { status: 204 }));
```
@dirkluijk
Copy link
Collaborator

The fix has been released in v0.4.1.

@gabscogna
Copy link
Author

Thank you @dirkluijk I updated here and everything works fine now with 0.4.1 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fix available
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants