Optional cross-fetch a.k.a. should libs include polyfills?

[maxmilton]

I'm really liking this projects simple aproach and small filesize. 80% of the time I have no need for a full-fledged client-side GraphQL framework so it's a great fit for most projects.

My only gripe is the included cross-fetch fetch polyfill. I'm of the school of thought that libraries should not include any polyfills and instead leave that up to the end developer. This prevents duplication of conflicting polyfills and gives the fexibility to swap out polyfill solutions as necessary. For modern/internal projects it's often not even necessary to use polyfills so it can avoid the performance penatlties of needing to parse the JS on load.

I'd like to propose removing cross-fetch and instead giving clear instructions in the documention about including a polyfill if legacy browser or node support is necessary.

[schickling]

I see your point. However, in most scenarios, the ease of use (and no need to install multiple packages) is more important.

I suggest we create another package that doesn't include polyfils (e.g. graphql-request-min)?

[maxmilton]

I don't disagree. Especially when you're targeting a large developer audience then it can make sense to make things as easy as possible.

Splitting it into a whole other package might be a little tedious to maintain. Perhaps there could be an additional file built in this project then we could do something like:

import { GraphQLClient } from 'graphql-request/dist/src/index.lite.js';

I'm sure it's possible to programmatically strip out the import 'cross-fetch/polyfill' line, which would make maintaining this much easier.

[davidrossjones]

I've just had to debug where global.fetch was being set in my node environment and tend to agree with @MaxMilton. This use of the fetch polyfill is not as transparent as perhaps it should be.

[ForsakenHarmony]

I'd have no polyfill as the default and maybe add something else for the polyfill

Also cross-fetch is huge in terms of file size

[robinvdvleuten]

The apollo fetch package allows one to override the fetch dependency. It would be nice if that's possible with this package as well. I am able to create a PR for this, if anyone likes the idea 🙃

[ForsakenHarmony]

apollo-fetch seems to still add cross-fetch to your bundle regardless

polyfills in libraries should be opt-in, not opt-out

[robinvdvleuten]

O but that's quite easy actually to do with webpack's null-loader;

{
    test: path.resolve(__dirname, 'node_modules/cross-fetch/dist/browser-polyfill.js'),
    use: 'null-loader'
},

[yuku]

I'm trying to use this package in my project which uses Rollup and TypeScript. With the build environment, cross-fetch's ./lib/index.es.js (Rollup prefers pkg.module over pkg.main) throws exception. Probably my rollup.config.js has a problem, but debugging a module bundler is really terrible... so I'm very happy if I can stop importing the cross-fetch.

[satya164]

The primary reason someone will be looking for this package instead of using larger all-purpose libraries like apollo is to reduce the bundle size (I actually am), and I think those developers will be aware of what a polyfill is and how to use it. Including the polyfill by default without any opt-out is certainly discouraging. I'm targeting modern browsers supporting fetch and I don't really want to include extra polyfills I don't need. Granted 2.5 kb isn't a lot, but if 4 libraries do this, it's extra 10 kb I don't need. I'll use null-loader for now. Thanks for the tip @robinvdvleuten

Why not document that it needs the fetch global to be present and include instructions on installing it? You can even include it along with graphql-request where it tells you how to install it.

[gr2m]

Hey @schickling and team, I’m considering to create https://github.com/octokit/graphql.js to be only a README file describing how to use graphql-request with GitHub’s GraphQL API. There would be no code whatsoever, just documentation. Because why reinvent the wheel :)

But depending on cross-fetch is a blocker, we do use node-fetch in our other Octokit libraries for e.g. the REST API and we will create a all-batteries-included octokit package, and the bundle size for browsers is important for us, as well as deduplication of dependencies. As cross-fetch has the node-fetch dependency pinned, that’s a problem which is out of your controll.

I’d suggest to rather document how to add the fetch polyfill yourself. fetch has now global browser support close to 90%, I think it’s time to optimise for them, and document a workaround for the minority of browsers.

I’m also happy to help maintain the library and make the transition :) Just let me know if you are interested

Update: I ended up building our own graphql library, it’s only 40 LOC :)

[ForsakenHarmony]

there's also https://www.npmjs.com/package/isomorphic-unfetch

[schickling]

This seems like the way to go @ForsakenHarmony. Is someone up for a PR on this?

@maticzav can you get this on the road? 🚀

[maticzav]

@schickling will add it to the list.

[cncolder]

cross-fetch cannot cover all scene.

In some special env. I need write a fetch adapter.

So I think the best choice is allow pass fetch ponyfill into GraphQLClient. Otherwise use global.fetch.

import fetch from './my-fetch-adapter'

const client = new GraphQLClient(endpoint, { fetch })

[tarekrached]

Being able to pass in our own fetch would be super useful. (As in, necessary in certain environments.) @robinvdvleuten 2.5 years ago you said you could put together a PR for this - you still up for it? 🙃

[jasonkuhrt]

Closed by https://github.com/prisma-labs/graphql-request/pull/212