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

support buffers when mocking responses #223

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

alexkolson
Copy link

@alexkolson alexkolson commented Nov 27, 2021

👋 @yinzara

This would add support for the issue referenced in #218. This would be really useful for me and perhaps others as well! Let me know if this looks okay and/or if I should implement in a different way. Thanks!

alexkolson added a commit to HamburgChimps/advanzia-assistant that referenced this pull request Nov 27, 2021
mock fetch manually using cross-fetch.Response and change it to jest-fetch-mock if/when jefflau/jest-fetch-mock#223 lands
Copy link

@andreasg123 andreasg123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, it looks like responseWrapper in src/index.js needs to be modified. That function creates a Response object via the constant ActualResponse that holds the JavaScript Response class. As the constructor for that class doesn't accept another Response object, the types of body and response in responseWrapper need to be checked to determine if that's already a Response object.

types/index.d.ts Outdated Show resolved Hide resolved
@alexkolson
Copy link
Author

@andreasg123 Took another shot at it. What do you thnk?

Copy link

@andreasg123 andreasg123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this wasn't the correct place to check for Response after all. I believe that this change in normalizeResponse should do it:

          return resp instanceof ActualResponse
            ? resp
            : typeof resp === 'string'
            ? responseWrapper(resp, init)
            : responseWrapper(resp.body, responseInit(resp, init))

@alexkolson
Copy link
Author

@andreasg123 Unfortunately if we swap out the check to go in normalizeResponse then it would have to be in two places no? Swapping the check to that location results in the test failing that checks that you can do:

fetchMock.mockResponse(new Response('foo'))

I am perhaps missing something and welcome your continued feedback. Sorry if I wasn't able to grasp what you were talking about on the first try!

@andreasg123
Copy link

@alexkolson, I'm concerned about these cases:

fetchMock.mockResponse(request => new Response('foo'));
fetchMock.mockResponse(request => Promise.resolve(new Response('foo')));

I don't think that those would work without the change that I suggested. For those examples, isFn(bodyOrFunction) in normalizeResponse would be true whereas your example wouldn't take that path.

@andreasg123
Copy link

As typeof resp === 'string' would be false, a new Response object would be constructed from resp.body, losing all the other information in the original Response object.

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