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

Make fetch implementation configurable #2008

Conversation

ctavan
Copy link

@ctavan ctavan commented Aug 7, 2017

Currently apollo-client relies on the globally available fetch api
which causes a number of problems. In the browser apollo-client uses a
polyfill which "pollutes" the global scope. This is something a library
should probably rather not do.

This change is based on previous discussion and work done in
#661 and #645.

Checklist:

  • If this PR is a new feature, please 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
  • If this was a change that affects the external API used in GitHunt-React, update GitHunt-React and post a link to the PR in the discussion.

Comments:

I believe that the current state where apollo-client inserts a fetch-polyfill 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 of is allowing to optionally pass a fetch implementation as configuration option when creating the network interface.

This PR still has some rough edges. My main concerns are:

  • Is fetch a good variable name for the config option?
  • The fetch option is currently defined as type any which should of course be changed.
  • The fetch-ponyfill type definitions could/should be improved, they are now just very generic.
  • Documentation is missing.
  • Changelog.

I'm curious about your feedback and I'll be happy fix the open points in case there's a chance to get this merged.

More references: #1134, #1155

@apollo-cla
Copy link

@ctavan: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@mention-bot
Copy link

@ctavan, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jbaxleyiii, @helfer and @Poincare to be potential reviewers.

@ctavan ctavan force-pushed the make-fetch-implementation-configurable branch from db0c796 to d99287a Compare August 7, 2017 15:20
@apollo-cla
Copy link

apollo-cla commented Aug 7, 2017

Messages
📖

Please add your name and email to the AUTHORS file (optional)

📖

If this was a change that affects the external API, please update the docs and post a link to the PR in the discussion

Generated by 🚫 dangerJS

@ctavan ctavan force-pushed the make-fetch-implementation-configurable branch 2 times, most recently from ce82e3b to 99b83c0 Compare August 7, 2017 16:02
Currently apollo-client relies on the globally available `fetch` api
which causes a number of problems. In the browser apollo-client uses a
polyfill which "pollutes" the global scope. This is something a library
should probably rather not do.

This change is based on previous discussion and work done in
apollographql#661 and apollographql#645.
@ctavan ctavan force-pushed the make-fetch-implementation-configurable branch from 99b83c0 to 5fe89c5 Compare August 7, 2017 16:03
@ctavan
Copy link
Author

ctavan commented Aug 7, 2017

Oh nose, I just discovered #1993 and https://github.com/apollographql/apollo-fetch which will make this one irrelevant…

I guess I'll take a closer look at apollo-fetch then as it seems that this also still globally imports isomorphic-fetch

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.

3 participants