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 createNetworkInterface isomorphic #645

Closed
gajus opened this issue Sep 13, 2016 · 14 comments
Closed

Make createNetworkInterface isomorphic #645

gajus opened this issue Sep 13, 2016 · 14 comments
Labels
📚 good-first-issue Issues that are suitable for first-time contributors. 🙏 help-wanted

Comments

@gajus
Copy link

gajus commented Sep 13, 2016

Calling createNetworkInterface on the server produces an error:

ReferenceError: fetch is not defined
   at HTTPFetchNetworkInterface.fetchFromRemoteEndpoint (/Users/gajuskuizinas/Documents/dev/node_modules/apollo-client/networkInterface.js:90:16)
[..]

The change required this to work would be simply replacing whatwg-fetch with isomorphic-fetch.

@stubailo
Copy link
Contributor

Is it enough to just document that people should install node-fetch on their server?

@gajus
Copy link
Author

gajus commented Sep 13, 2016

That would require me to pollute my global namespace with fetch keyword.

@stubailo
Copy link
Contributor

Apollo already does that for you on the client :] that's how the polyfill works, and one of the reasons we didn't include the polyfill on the server.

I'd be open to replacing fetch entirely with XHR/request, but we couldn't find any polyfills that don't add global stuff.

@stubailo
Copy link
Contributor

Related: #269

Perhaps you'd be interested in working on this?

@gajus
Copy link
Author

gajus commented Sep 14, 2016

I'd be open to replacing fetch entirely with XHR/request, but we couldn't find any polyfills that don't add global stuff.

There is nothing wrong with using fetch. Whats wrong is that you are trying to polyfill it.

Libraries are not meant to polyfill anything. Only application can polyfill its environment.

What you should be doing is simply adding isomorphic-fetch as a dependency and using it as a dependency, e.g.

const fetch = require('isomorphic-fetch');

Frontend users, who are concerned with the bundle size and who are already using another fetch implementation, are able to to dedupe the dependency at the bundle step.

I am happy to raise a PR if you are happy with this proposal.

@stubailo
Copy link
Contributor

As far as I know isomorphic fetch still adds a globa symbol if you require it that way, but I'm definitely open to a PR to make that change.

@gajus
Copy link
Author

gajus commented Sep 14, 2016

As far as I know isomorphic fetch still adds a globa symbol if you require it that way

/me rolls up shirt sleeves

matthew-andrews/isomorphic-fetch#41

@gajus
Copy link
Author

gajus commented Sep 14, 2016

As far as I know isomorphic fetch still adds a globa symbol if you require it that way

Thanks for bringing this to my attention. I wasn't aware of it.

As their own docs state:

This module wraps the github/fetch polyfill in a CommonJS module for browserification, and avoids appending anything to the window, instead returning a setup function when fetch-ponyfill is required. Inspired by object-assign.

– https://github.com/matthew-andrews/isomorphic-fetch

I will raise a PR that utilises https://github.com/qubyte/fetch-ponyfill.

@stubailo
Copy link
Contributor

Yeah, I guess the idea of a "polyfill" is that it reproduces the native functionality. In this case, when browsers support it, fetch is a global function just like any other native browser feature.

does the ponyfill work on Node?

@jamiter
Copy link
Contributor

jamiter commented Sep 15, 2016

does the ponyfill work on Node?

When used in Node, delegates to node-fetch instead.

@sedubois
Copy link

sedubois commented Nov 11, 2016

Agreed, would be great not to have to do import 'isomorphic-fetch'; just to get Apollo working server-side. IMHO Apollo shouldn't rely on global variables.

@sedubois
Copy link

sedubois commented Nov 11, 2016

How about something like this?

const fetch = global.fetch || (typeof window === 'undefined' 
  ? require('node-fetch') 
  : require('whatwg-fetch'));

And adding node-fetch in your package.json.

@ryancole
Copy link

In the meantime, would it just be enough to create a custom network interface that uses fetch ponyfill?

@stale
Copy link

stale bot commented Jul 29, 2017

This issue has been automatically closed because it has not had recent activity after being marked as stale. If you belive this issue is still a problem or should be reopened, please reopen it! Thank you for your contributions to Apollo Client!

ctavan added a commit to ctavan/apollo-client that referenced this issue 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
apollographql#661 and apollographql#645.
ctavan added a commit to ctavan/apollo-client that referenced this issue 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
apollographql#661 and apollographql#645.
ctavan added a commit to ctavan/apollo-client that referenced this issue 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
apollographql#661 and apollographql#645.
ctavan added a commit to ctavan/apollo-client that referenced this issue 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
apollographql#661 and apollographql#645.
ctavan added a commit to ctavan/apollo-client that referenced this issue 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
apollographql#661 and apollographql#645.
ctavan added a commit to ctavan/apollo-client that referenced this issue 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
apollographql#661 and apollographql#645.
ctavan added a commit to ctavan/apollo-client that referenced this issue 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
apollographql#661 and apollographql#645.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
📚 good-first-issue Issues that are suitable for first-time contributors. 🙏 help-wanted
Projects
None yet
Development

No branches or pull requests

7 participants