jefflau / jest-fetch-mock

Jest mock for fetch
MIT License
886 stars 117 forks source link

added async support #75

Closed lsimone closed 5 years ago

lsimone commented 6 years ago

I needed async mocks in order to test other use cases (timeouts etc.) so I wrote this PR

jefflau commented 6 years ago

Hi @lsimone,

Thanks for the PR

Could you add simple, but standalone test example to the README to show how you might use it with promises?

lsimone commented 6 years ago

Hi @jefflau, I already did it :) My commit contains the README modification. Let me know if anything is not clear.

Thanks!

jefflau commented 6 years ago

I saw the readme. I wanted to see a more comprehensive example so i could understand the usecase for your addition.

Do you have a repo that i can see where you use it?

lsimone commented 6 years ago

sure @jefflau : this is the repo where I need to test the timeout handler, for instance. I need to emulate the server delay, so I need to use a promise instead an object as server response.

Let me know if anything is not clear.

jefflau commented 6 years ago

Just want to keep the discussion open for the best possible API before I decide on merging the PR for async support. What's the advantages of taking a promise as an argument vs taking a function? @tiger9502 You initially suggested adding a function - any reason why you might do one over the other?

@lsimone I looked at your repo and it seems your main use case is also for arbitrary delays. If we did something to what @tiger9502 and just build a delay functionality into the mock and just take a delay in milliseconds and hide all the async implementation inside the library do you think that would also satisfy your needs or were you using the return of promise for other things as well?

lsimone commented 6 years ago

@jefflau at the beginning I thought about a delay-only functionality, then I thought it was just limiting the use cases my PR was covering...because with a promise you can do any async operation, not just delaying. Then I guess we can also add to the API the delay option, but just as syntactic sugar....basically I would not limit the library power just to save one user's code line.

Maybe a better API would be accepting a function like this one: function resolver() : Promise<Response> where Response is an object {body, init}

The lib usage would be in this case: fetch.mockResponse(() => callMyAsyncApi().then(res => ({body: res.data})))

Using a function, we know that it will be executed just when the mock server need to respond to a call, while my implementation that uses a Promise, force the async operation to run as it is declared. My implemtation would execute directly callMyAsyncApi() before the mocked call is performed: fetch.mockResponse(callMyAsyncApi().then(res => ({body: res.data}))

So this is the enhancement I would do to the code I pushed.

These are my 2 cents :-)

jefflau commented 6 years ago

@lsimone That sounds great. Do you think there is any benefit to testing whether or not the function's return is a promise or not and allowing the user to return a Response Object without using a promise?

lsimone commented 6 years ago

@jefflau you mean something like this?

/**
 * @callback resolver
 * @return {{body: Object, [init]: Object}} response
 */

/**
 * @param {Object | resolver} bodyOrResolver
 * @param {Object} [init]
 */
mockResponse(bodyOrResolver, init);
jefflau commented 6 years ago

Im not sure i quite understand the syntax. But basically it can take a function or a response object. And that function can return either a promise which returns a response object OR a response object without a promise

lsimone commented 6 years ago

The proposal is:

bodyOrResolver can be an object (body, as before, retro-compatible) or a function resolver.

resolver should return either

WDYT?

jefflau commented 6 years ago

Looks perfect to me.

jefflau commented 6 years ago

Any update on this?

lsimone commented 5 years ago

I want to provide the new PR but I didn't have time so far. I'll try to find it in the next 2 weeks!

lsimone commented 5 years ago

@jefflau I pushed my last commit and PR has been updated. Let me know if it looks good to you! thanks

jefflau commented 5 years ago

Since your first commit in June, I have made some changes and included tests in the repo. Could you please merge with master and write some tests for your new features?

Thanks a lot for the hardwork!

lsimone commented 5 years ago

ok i'll merge your changes into my repo and update the PR

jefflau commented 5 years ago

Thanks!

lsimone commented 5 years ago

ok @jefflau I think it should be ok now. I removed the IDE settings file, modified README, fixed an issue, and added 2 tests too.

Let me know and have a great weekend!

jefflau commented 5 years ago

Thanks a lot @lsimone, I'll take a look over the weekend sometime and hopefully get this merged in by next week!