-
Notifications
You must be signed in to change notification settings - Fork 194
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
Conversation
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.
This approach in the most API-preserving way, which I really forgot considering about. Replace the |
This is not needed anymore if #77 is merged. You can then just override the global fetch. |
@cevou but what if I want to wrap the fetch function for just this module and not globally (which is my use case)? |
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? |
@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. |
I wrap the fetch function because I want to do some general handling before
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 |
@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. |
Another way to do this in a non-breaking way is to add a function to the exported middleware function.
|
The correct way for an external library to use 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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@MrOggy85, can you then merge it? I don't have write access to the repo. |
Closing, as we are deprecating our use of this middleware. |
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.