Skip to content
This repository has been archived by the owner on Aug 20, 2020. It is now read-only.

Use fetch-ponyfill instead of isomorphic-fetch #23

Closed
wants to merge 1 commit into from

Conversation

ctavan
Copy link
Contributor

@ctavan ctavan commented Aug 7, 2017

Use a fetch ponyfill instead of modifying the global scope and
polyfilling the fetch API.

Closes #22.

I believe that the current state where apollo-fetch imports a isomorphic-fetch and thus modifies the global scope is a behavior that a library should not have. I would like to use apollo-client within another library myself and I do not want to pollute the globale scope of they sites who will use my library.

The most straightforward change I can think is to use fetch-ponyfill internally by default which should have essentially the same effect than isomorphic-fetch since it also uses either node-fetch on the server or whatwg-fetch on the browser under the hood, just like isomorphic-fetch does.

TODO:

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change
  • Add your name and email to the AUTHORS file (optional)
  • If this was a change that affects the external API, update the docs and post a link to the PR in the discussion
  • The fetch-ponyfill type definitions could/should be improved, they are now just very generic.

References:

@jbaxleyiii
Copy link
Contributor

@ctavan another potential option here is to assume fetch is present. We have gone back in forth a number of times with Apollo things. I do agree this is a better first step towards removing fetch as a dependency though

@ctavan
Copy link
Contributor Author

ctavan commented Aug 7, 2017

@jbaxleyiii assuming fetch is present (plus the already existing option of being able to pass customFetch) would be equally fine for me.

I would even prefer that variant since I think it's more future-proof and would allow to keep the library code of apollo-fetch cleaner.

I guess it's up to you to decide whether this breaking change can be forced upon the apollo-client users…

I think the major argument against it was, that people could not notice this during development because they typically use a modern browser and not IE11. However I would argue that it is rather the duty of the developer who uses apollo-client in their project to satisfy the requirements and test the final code, than the task of the library to cover legacy browsers.

After all people who can afford to build their projects only for modern browsers would even benefit from not having a fetch polyfill or ponyfill strictly bundled into apollo-client.

So from my side: big 👍 for simply expecting fetch to be present and printing a warning otherwise.

@jbaxleyiii
Copy link
Contributor

jbaxleyiii commented Aug 7, 2017

@ctavan I think with the 2.0 we may be able to make that the case since we are moving to links fully.

Lets think on this a bit before closing in that favor though

@ctavan
Copy link
Contributor Author

ctavan commented Aug 11, 2017

@jbaxleyiii since apollographql/apollo-client#1941 looks like it's getting more and more mature, I wanted to warm up the discussion here again.

Has there been more discussion on the apollo-team side w.r.t. dropping the fetch polyfill in 2.0? The more I think about it the more I believe this should really be done.

For those who want the old behavior we could simply provide an additional apollo-fetch-polyfilled module (similar to apollo-fetch-upload) as a drop-in alternative to apollo-fetch, which should come without the polyfill and simply assume the fetch api to be present. User who have to support older browsers and want to use a ponyfill can then still make use of the customFetch option.

Given that anyone who upgrades to apollo-client 2.0 will have to upgrade the initialization code to use apollo-fetch anyways, I think this major version bump is the best opportunity to make this change and get rid of the global scope pollution.

@ctavan
Copy link
Contributor Author

ctavan commented Aug 11, 2017

@jbaxleyiii FYI I gave it a try in #30. Let me know what you think.

Use a fetch ponyfill instead of modifying the global scope and
polyfilling the fetch API.

Closes apollographql#22.
@ctavan
Copy link
Contributor Author

ctavan commented Oct 4, 2017

I believe that bundling a ponyfill doesn't make any sense. I think it makes much more sense to require fetch api to be present in the default case and provide a convenience apollo-fetch-polyfill module which includes the polyfill.

See #30

@ctavan ctavan closed this Oct 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants