Skip to content

Allow a fetch function to be passed into apiMiddleware #58

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

Closed
wants to merge 3 commits into from
Closed

Allow a fetch function to be passed into apiMiddleware #58

wants to merge 3 commits into from

Conversation

nwwells
Copy link

@nwwells nwwells commented Mar 22, 2016

This was done in the most API-preserving way possible. Another (breaking) approach would be to force the library user to initialize the apiMiddleware with the given fetch. I'd actually prefer that, but thought that preserving the existing API would be the most important priority right now.

Note also that this way of doing it is slightly leaky as passing in fetch essentially modifies a global-ish variable. The test is actually quite leaky, since any test after it will be executed with the mocked fetch.

This was the result of some rapid prototyping. I just want to see if the idea has merit, and get some guidance on the right direction before getting too far into either approach.

nwwells added 2 commits March 22, 2016 12:44
This was done in the most API-preserving way possible. Another (breaking)
change would be to force the library user to initialize the apiMiddleware
with the given fetch. I'd actually prefer that, but thought that preserving
the existing API would be the most important priority right now.

Note also that this way of doing it is slightly leaky as passing in fetch
essentially modifies a global-ish variable. The test is actually quite
leaky, since any test after it will be executed with the mocked fetch.

This was the result of some rapid prototyping. I just want to see if the
idea has merit.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5d8390a on symbiont-io:master into 1543910 on agraboso:master.

@Darmody
Copy link

Darmody commented Mar 24, 2016

This approach in the most API-preserving way, which I really forgot considering about.

Replace the fetch function can resolve my problem, but it looks heavily for just add some hooks for the middleware, I mean, for a package user.

@cevou
Copy link

cevou commented May 10, 2016

This is not needed anymore if #77 is merged. You can then just override the global fetch.

@nwwells
Copy link
Author

nwwells commented May 10, 2016

@cevou but what if I want to wrap the fetch function for just this module and not globally (which is my use case)?

@cevou
Copy link

cevou commented May 10, 2016

You can easily override global fetch before your code is executed and reset it afterwards. This however only works if no other module calls the fetch function in between. The question is why you are wrapping a standard API function in your module?

@mlippens
Copy link

@cevou then you are patching a global twice just for one use case? Let's assume that we follow @nwwells approach here, does it really have a downside but not following the fetch spec and providing a hook in the middleware? If I turn it around, to me the only reason to choose #77 is that it is conform with the fetch spec.

@nwwells
Copy link
Author

nwwells commented May 11, 2016

I wrap the fetch function because I want to do some general handling before redux-api-middleware sees the result. I don't modify the response at all, and my wrapping function maintains the fetch interface.

why you are wrapping a standard API function in your module?

Because wrapping is better than patching. Because modifying the global object is dangerous. Because assuming that a given function executes synchronously is generally a bad idea, and because it just doesn't work in this case specifically (the only obvious way to do it would be to override the headers CALL_API action function, but then I don't have a way of reseting it).

@cevou
Copy link

cevou commented May 11, 2016

@froginvasion I think you should not patch the global function at all. Especially, if you don't want to change the actual functionality but rather do some preprocessing of the result.
Wouldn't it be better to have some preprocessing hook/callback that you could just pass in the CALL_API action that is then executed before the result is passed on?

@alexanderchr
Copy link

Another way to do this in a non-breaking way is to add a function to the exported middleware function.

apiMiddleware.withFetch(fetchImplementation)

See reduxjs/redux-thunk#70

@migueloller
Copy link

migueloller commented Jun 29, 2016

The correct way for an external library to use fetch is to either allow the developer to provide their own polyfill in the global (This is the best option since this is the behavior that will happen once the standard is implemented. And it's already implemented in Chrome and React Native) or let the developer pass it as an initialization parameter (This is less desirable but still compliant).

The current way causes a whole slew of problems as we can see being discussed here, and the mocking and React Native issues discussed in #77.

@teofilomonteiro
Copy link

So when is this going to be release? I think it will be a great improvement since we will be able to mock the fetch request.

Copy link

@MrOggy85 MrOggy85 left a comment

Choose a reason for hiding this comment

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

LGTM

@nwwells
Copy link
Author

nwwells commented Jan 23, 2017

@MrOggy85, can you then merge it? I don't have write access to the repo.

@coveralls
Copy link

coveralls commented Feb 8, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 6d5cc7a on symbiont-io:master into 1543910 on agraboso:master.

@nwwells
Copy link
Author

nwwells commented Feb 8, 2017

Closing, as we are deprecating our use of this middleware.

@nwwells nwwells closed this Feb 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants