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

Issue 75: Fetch cannot be mocked #77

Closed

Conversation

farmisen
Copy link

Came here to report it and saw @cevou did it a few days ago (#75). So here is a very simple PR to fix this problem.

Stop importing fetch from isomorphic-fect to rely on the globally registered fetch function.

@cevou
Copy link

cevou commented May 10, 2016

Thanks for creating the PR. Hope this gets merged soon.

@coveralls
Copy link

coveralls commented May 10, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling baf3b4c on farmisen:issue_75_Fetch_cannot_be_mocked into e72ac27 on agraboso:master.

@nwwells
Copy link

nwwells commented May 10, 2016

Not a fan: depending on globals is an anti-pattern

@cevou
Copy link

cevou commented May 10, 2016

By definition the Fetch API provides a global function.

It (...) provides a global fetch() method that provides an easy, logical way to fetch resources asynchronously across the network.

https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch
https://fetch.spec.whatwg.org/

@benpickles
Copy link

I wonder whether requiring a fetch polyfill at all should be left to the user - for instance I'm already using whatwg-fetch as I haven't yet implemented server-side rendering. Either way my app may only require supporting modern browsers in which case this just adds to the download weight.

In which case isomorphic-fetch should be moved to devDependencies as it's then only required for testing.

@alexanderchr
Copy link

Definitely, it should be up to the user to supply their favourite fetch polyfill. A library should be indifferent to whether that happens to be whatwg-fetch, node-fetch or a native browser implementation.

An option to avoid globals would be to allow the user to supply a fetch implementation when creating the middleware.

@nwwells
Copy link

nwwells commented May 16, 2016

@alexanderchr see #58

@alexanderchr
Copy link

@nwwells Right, way ahead of me. Sorry for not reading before typing.

And it is indeed a much better solution. What is blocking it from being merged? @agraboso

@agraboso
Copy link
Owner

I much prefer #58 (so I'm closing this — thanks though, @farmisen). I probably won't merge that one either though because I'm thinking of allowing an object of options at creation time, which would allow not only a custom fetch but also other customizations. One of those has already been requested in #62 (allowing a root URL, which I find more than reasonable). But there's the possibility that somebody will come up with a valid reason for further customization, so I'd rather have a way of adding it without changing the API for those people that don't need it.

@migueloller
Copy link

Hey @agraboso, how far away are we from having an object of options at creation time?

@seethroughdev
Copy link

There are several issues and PRs offering solutions. Is there any progress towards this? Building @farmisen version works fine for me in testing.

@agraboso can you post an update with your thoughts?

@adam8810
Copy link

What is the status of the repo? It seems to be pretty stagnant now.

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.

10 participants