IanVS / vitest-fetch-mock

Vitest mock for fetch, forked from jest-fetch-mock
MIT License
68 stars 9 forks source link

feat: allow more types of overloads #26

Closed dirkluijk closed 1 month ago

dirkluijk commented 1 month ago

This PR makes it possible to use more types of overloaded functions:

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 https://github.com/IanVS/vitest-fetch-mock/issues/25#issuecomment-2438707468:

fetch.mockResponseOnce({ status: 204 });
fetch.mockResponseOnce(new Response(null, { status: 204 }));
IanVS commented 1 month ago

Hmmmm, I'm not sure if I love the idea of even more ways to accomplish the same thing, I think this project already has too many overloads already. I might even suggest that we move towards a future where a Response needs to be used, either on its own, or in a function that returns a Response or the promise of a Response. It's a bit more annoying to use that way, but it's easier to document and learn, and there's no ambiguity of what's going on behind the scenes.

But I see that today, the only time a Response can be provided is when returned in a function. So this PR would be a necessary step anyway. I think this makes sense to do for now, and hopefully we can find a way to deprecate simple strings and objects being passed to mockResponse-style methods.

Do you think there are any docs that should be changed if we merge this feature?

dirkluijk commented 1 month ago

Hmmmm, I'm not sure if I love the idea of even more ways to accomplish the same thing, I think this project already has too many overloads already. I might even suggest that we move towards a future where a Response needs to be used, either on its own, or in a function that returns a Response or the promise of a Response. It's a bit more annoying to use that way, but it's easier to document and learn, and there's no ambiguity of what's going on behind the scenes.

Agreed. I'd say we could narrow it down by limiting the current definition:

type ResponseProvider = ResponseLike | ((request: Request) => ResponseLike | Promise<ResponseLike>);
type ResponseLike = MockResponse | ResponseBody | Response;
type ResponseBody = string;

down to:

type ResponseProvider = ResponseLike | ((request: Request) => ResponseLike | Promise<ResponseLike>);
type ResponseLike = ResponseBody | Response;
type ResponseBody = string;

or even

type ResponseProvider = Response | ((request: Request) => Response | Promise<Response>);

although I see the added value of keeping the response body overload.

Do you think there are any docs that should be changed if we merge this feature?

It's a superset of the current API, so I wouldn't mind just keeping it as-is. But I am also in favor of rewriting the whole docs for the 1.0 version, when we introduce breaking changes. My ideas for 1.0 would be: