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

Doesn't work in service workers #216

Closed
pimterry opened this issue Oct 6, 2020 · 6 comments · Fixed by #597
Closed

Doesn't work in service workers #216

pimterry opened this issue Oct 6, 2020 · 6 comments · Fixed by #597

Comments

@pimterry
Copy link

pimterry commented Oct 6, 2020

This isn't strictly speaking a graphql-request bug, but it's a bug in cross-fetch that makes graphql-request unusable in my case. See lquixada/cross-fetch#78. In short:

  • Cross-fetch uses XMLHttpRequest to ponyfill Fetch in browser-like environments (even if fetch is available, as far as I can tell).
  • Service workers have Fetch available, and are real browser environments, but don't have XMLHttpRequest.
  • Because of that, GraphQL requests made with this library in my service worker all fail with:
    ReferenceError: XMLHttpRequest is not defined
      at browser-ponyfill.js:463
      at new Promise (<anonymous>)
      at Object.fetch [as default] (browser-ponyfill.js:455)
      at GraphQLClient.<anonymous> (index.ts:87)
      at step (createRequestBody.ts:45)
      at Object.next (createRequestBody.ts:45)
      at createRequestBody.ts:45
      at new Promise (<anonymous>)
      at createRequestBody.ts:45
      at GraphQLClient.request (index.ts:73)
    

As far as I can tell there's no practical workarounds right now, it's just unusable.

Fixing cross-fetch would resolve this, or alternatively graphql-request could use the built-in fetch method directly, without cross-fetch, if it detects that it's already available in the local environment.

@lopezjurip
Copy link

For a quick workaround I was able to make it work by passing the global fetch instance to the constructor:

import { GraphQLClient } from "graphql-request";
const client = new GraphQLClient("https://localhost:3000/graphql", { fetch: fetch });

@patdx
Copy link

patdx commented Aug 30, 2022

Can cross-fetch be removed entirely?

I'm trying to build a project to deploy to cloudflare workers which has a strict 1MB limit, so even if the workaround above is possible, it's not ideal because cross-fetch is still getting included in the final bundle and taking up precious space (1MB is quite small...).

Interestingly, this can be overridden fairly easily in webpack based bundles, as is done inside Next.js:

https://github.com/vercel/next.js/blob/6da621df635304615783b2442b002f0618199796/packages/next/build/webpack-config.ts#L294

However the project I am working on is Vite-based, and I can't figure out the correct incantation to stub out the unneeded cross-fetch code in Vite.

I think ky has a nice approach, the basic package has a pure fetch wrapper and there is an additional ky-universal package which bundles node-fetch:

In a similar style, graphql-request could offer two exports for easy migration, like:

import { GraphQLClient } from "graphql-request"; // "pure" version
import { GraphQLClient } from "graphql-request/universal"; // wrapped with cross-fetch

sveltejs/kit#1674 (comment)

Hopefully libraries will stop bundling things like cross-fetch, now that more and more runtimes expose it natively

For now I ended up applying a patch using pnpm:

// package.json
{
  // ...
  "pnpm": {
    "patchedDependencies": {
      "[email protected]": "patches/[email protected]"
    }
  }
}
# patches/[email protected]
diff --git a/dist/index.js b/dist/index.js
index 43debc9550f4c19dadd7b9e78ae1078c9993e7f2..792b130d5ec630bc368c8c188063b005083ba2dd 100644
--- a/dist/index.js
+++ b/dist/index.js
@@ -81,7 +81,6 @@ var __importDefault = (this && this.__importDefault) || function (mod) {
 };
 Object.defineProperty(exports, "__esModule", { value: true });
 exports.GraphQLWebSocketClient = exports.gql = exports.resolveRequestDocument = exports.batchRequests = exports.request = exports.rawRequest = exports.GraphQLClient = exports.ClientError = void 0;
-var cross_fetch_1 = __importStar(require("cross-fetch")), CrossFetch = cross_fetch_1;
 var parser_1 = require("graphql/language/parser");
 var printer_1 = require("graphql/language/printer");
 var createRequestBody_1 = __importDefault(require("./createRequestBody"));
@@ -96,7 +95,7 @@ var resolveHeaders = function (headers) {
     var oHeaders = {};
     if (headers) {
         if ((typeof Headers !== 'undefined' && headers instanceof Headers) ||
-            (CrossFetch && CrossFetch.Headers && headers instanceof CrossFetch.Headers)) {
+            (globalThis.Headers && headers instanceof globalThis.Headers)) {
             oHeaders = HeadersInstanceToPlainObject(headers);
         }
         else if (Array.isArray(headers)) {
@@ -217,7 +216,7 @@ var GraphQLClient = /** @class */ (function () {
             var rawRequestOptions, _a, headers, _b, fetch, _c, method, requestMiddleware, responseMiddleware, fetchOptions, url, operationName;
             return __generator(this, function (_d) {
                 rawRequestOptions = parseArgs_1.parseRawRequestArgs(queryOrOptions, variables, requestHeaders);
-                _a = this.options, headers = _a.headers, _b = _a.fetch, fetch = _b === void 0 ? cross_fetch_1.default : _b, _c = _a.method, method = _c === void 0 ? 'POST' : _c, requestMiddleware = _a.requestMiddleware, responseMiddleware = _a.responseMiddleware, fetchOptions = __rest(_a, ["headers", "fetch", "method", "requestMiddleware", "responseMiddleware"]);
+                _a = this.options, headers = _a.headers, _b = _a.fetch, fetch = _b === void 0 ? globalThis.fetch : _b, _c = _a.method, method = _c === void 0 ? 'POST' : _c, requestMiddleware = _a.requestMiddleware, responseMiddleware = _a.responseMiddleware, fetchOptions = __rest(_a, ["headers", "fetch", "method", "requestMiddleware", "responseMiddleware"]);
                 url = this.url;
                 if (rawRequestOptions.signal !== undefined) {
                     fetchOptions.signal = rawRequestOptions.signal;
@@ -256,7 +255,7 @@ var GraphQLClient = /** @class */ (function () {
         }
         var variables = variablesAndRequestHeaders[0], requestHeaders = variablesAndRequestHeaders[1];
         var requestOptions = parseArgs_1.parseRequestArgs(documentOrOptions, variables, requestHeaders);
-        var _a = this.options, headers = _a.headers, _b = _a.fetch, fetch = _b === void 0 ? cross_fetch_1.default : _b, _c = _a.method, method = _c === void 0 ? 'POST' : _c, requestMiddleware = _a.requestMiddleware, responseMiddleware = _a.responseMiddleware, fetchOptions = __rest(_a, ["headers", "fetch", "method", "requestMiddleware", "responseMiddleware"]);
+        var _a = this.options, headers = _a.headers, _b = _a.fetch, fetch = _b === void 0 ? globalThis.fetch : _b, _c = _a.method, method = _c === void 0 ? 'POST' : _c, requestMiddleware = _a.requestMiddleware, responseMiddleware = _a.responseMiddleware, fetchOptions = __rest(_a, ["headers", "fetch", "method", "requestMiddleware", "responseMiddleware"]);
         var url = this.url;
         if (requestOptions.signal !== undefined) {
             fetchOptions.signal = requestOptions.signal;
@@ -288,7 +287,7 @@ var GraphQLClient = /** @class */ (function () {
     };
     GraphQLClient.prototype.batchRequests = function (documentsOrOptions, requestHeaders) {
         var batchRequestOptions = parseArgs_1.parseBatchRequestArgs(documentsOrOptions, requestHeaders);
-        var _a = this.options, headers = _a.headers, _b = _a.fetch, fetch = _b === void 0 ? cross_fetch_1.default : _b, _c = _a.method, method = _c === void 0 ? 'POST' : _c, requestMiddleware = _a.requestMiddleware, responseMiddleware = _a.responseMiddleware, fetchOptions = __rest(_a, ["headers", "fetch", "method", "requestMiddleware", "responseMiddleware"]);
+        var _a = this.options, headers = _a.headers, _b = _a.fetch, fetch = _b === void 0 ? globalThis.fetch : _b, _c = _a.method, method = _c === void 0 ? 'POST' : _c, requestMiddleware = _a.requestMiddleware, responseMiddleware = _a.responseMiddleware, fetchOptions = __rest(_a, ["headers", "fetch", "method", "requestMiddleware", "responseMiddleware"]);
         var url = this.url;
         if (batchRequestOptions.signal !== undefined) {
             fetchOptions.signal = batchRequestOptions.signal;
diff --git a/src/index.ts b/src/index.ts
index 0758ec3779556ea08730cb8b2efa3c8cc9401e32..07ba1739b062c86c236ce85ff5134dd4f8222626 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -1,4 +1,3 @@
-import crossFetch, * as CrossFetch from 'cross-fetch'
 import { TypedDocumentNode } from '@graphql-typed-document-node/core'
 import { OperationDefinitionNode, DocumentNode } from 'graphql/language/ast'

@patdx
Copy link

patdx commented Aug 31, 2022

Update: I ran into a similar issue with the form-data dependency, it was pulling in a lot of node.js dependencies which caused my project to fail to build for cloudflare workers.

After some more research, it seems that the philosophy of this project is strongly leaning towards always including the Node.js polyfills:

#40
#88
#127

So to have this package meet my use case, both the cross-fetch and form-data dependencies would have to be made optional (and maybe others). Seems a bit complicated to change so maybe not for my use case. Thanks!

@jimmywarting
Copy link

I suggest that you let users polyfill this kinds of things that they need by themself.
then they can optionally install other dependencies that they need and the pkg would be much smaller.

@richardscarrott
Copy link

Hi @patdx -- I know this was a year ago, but we're also deploying to cloudflare workers and I wondered if you also looked at getting rid of the graphql dependency which is ~40kb?

@patdx
Copy link

patdx commented Sep 1, 2023

@richardscarrott Wow, time flies 😮 The project I was working on at the time also had a graphql server, where I think the graphql dependency is true requirement(?) so it didn't cross my mind to get rid of it.

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 a pull request may close this issue.

5 participants