Repository: FormidableLabs/urql
Pull request #678 (2020-04-01 17:18:35)

Fix non-2xx GraphQL responses on fetch exchanges
## Summary

This always parses JSON data in all fetch exchanges and falls back to generating errors when it's safe to do so, i.e. when the data isn't GraphQL data. When we can't parse the response we also fall back to our previous error logic, but we also account for `new Error('No Content')` in case of success scenarios.

`4.43 kB -> 4.46kB`

## Set of changes

- Update `executeFetch` in `fetchExchange`
- Update `executeFetch` in `multipartFetchExchange`


changeset-bot[bot]: ###  💥  No Changeset

Latest commit: 3866536e35a69f2843e72093393e5f2f0349b10f

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

**If these changes should be published to npm, you need to add a changeset.**

<details><summary>This PR includes no changesets</summary>

  When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

</details>

[Click here to learn what changesets are, and how to add one](https://github.com/changesets/changesets/blob/master/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add a changeset to this PR](https://github.com/FormidableLabs/urql/new/fix/always-parse-json?filename=.changeset/bright-waves-admire.md&value=---%0A%22%40urql%2Fcore%22%3A%20patch%0A%22%40urql%2Fexchange-multipart-fetch%22%3A%20patch%0A---%0A%0AFix%20non-2xx%20GraphQL%20responses%20on%20fetch%20exchanges%0A)



Patch:
diff --git a/.changeset/rotten-jobs-count.md b/.changeset/rotten-jobs-count.md
new file mode 100644
index 00000000..cd4522d3
--- /dev/null
+++ b/.changeset/rotten-jobs-count.md
@@ -0,0 +1,6 @@
+---
+'@urql/exchange-multipart-fetch': patch
+'@urql/core': patch
+---
+
+Fix non-2xx results never being parsed as GraphQL results. This can result in valid GraphQLErrors being hidden, which should take precedence over generic HTTP NetworkErrors.
diff --git a/exchanges/multipart-fetch/src/multipartFetchExchange.ts b/exchanges/multipart-fetch/src/multipartFetchExchange.ts
index 317afed6..84b1c618 100644
--- a/exchanges/multipart-fetch/src/multipartFetchExchange.ts
+++ b/exchanges/multipart-fetch/src/multipartFetchExchange.ts
@@ -166,24 +166,31 @@ const executeFetch = (
   opts: RequestInit
 ): Promise<OperationResult> => {
   const { url, fetch: fetcher } = operation.context;
-  let response: Response | undefined;
+  let statusNotOk = false;
+  let response: Response;
 
   return (fetcher || fetch)(url, opts)
-    .then(res => {
-      const { status } = res;
-      const statusRangeEnd = opts.redirect === 'manual' ? 400 : 300;
+    .then((res: Response) => {
       response = res;
-
-      if (status < 200 || status >= statusRangeEnd) {
-        throw new Error(res.statusText);
-      } else {
-        return res.json();
+      statusNotOk =
+        res.status < 200 ||
+        res.status >= (opts.redirect === 'manual' ? 400 : 300);
+      return res.json();
+    })
+    .then((result: any) => {
+      if (!('data' in result) && !('errors' in result)) {
+        throw new Error('No Content');
       }
+
+      return makeResult(operation, result, response);
     })
-    .then(result => makeResult(operation, result, response))
-    .catch(err => {
-      if (err.name !== 'AbortError') {
-        return makeErrorResult(operation, err, response);
+    .catch((error: Error) => {
+      if (error.name !== 'AbortError') {
+        return makeErrorResult(
+          operation,
+          statusNotOk ? new Error(response.statusText) : error,
+          response
+        );
       }
     });
 };
diff --git a/packages/core/src/exchanges/fetch.ts b/packages/core/src/exchanges/fetch.ts
index dbd089e0..dca0f4f6 100755
--- a/packages/core/src/exchanges/fetch.ts
+++ b/packages/core/src/exchanges/fetch.ts
@@ -141,24 +141,31 @@ const executeFetch = (
   opts: RequestInit
 ): Promise<OperationResult> => {
   const { url, fetch: fetcher } = operation.context;
-  let response: Response | undefined;
+  let statusNotOk = false;
+  let response: Response;
 
   return (fetcher || fetch)(url, opts)
-    .then(res => {
-      const { status } = res;
-      const statusRangeEnd = opts.redirect === 'manual' ? 400 : 300;
+    .then((res: Response) => {
       response = res;
-
-      if (status < 200 || status >= statusRangeEnd) {
-        throw new Error(res.statusText);
-      } else {
-        return res.json();
+      statusNotOk =
+        res.status < 200 ||
+        res.status >= (opts.redirect === 'manual' ? 400 : 300);
+      return res.json();
+    })
+    .then((result: any) => {
+      if (!('data' in result) && !('errors' in result)) {
+        throw new Error('No Content');
       }
+
+      return makeResult(operation, result, response);
     })
-    .then(result => makeResult(operation, result, response))
-    .catch(err => {
-      if (err.name !== 'AbortError') {
-        return makeErrorResult(operation, err, response);
+    .catch((error: Error) => {
+      if (error.name !== 'AbortError') {
+        return makeErrorResult(
+          operation,
+          statusNotOk ? new Error(response.statusText) : error,
+          response
+        );
       }
     });
 };


Test patch:
diff --git a/exchanges/multipart-fetch/src/multipartFetchExchange.test.ts b/exchanges/multipart-fetch/src/multipartFetchExchange.test.ts
index ed3c3244..d25b2f9f 100644
--- a/exchanges/multipart-fetch/src/multipartFetchExchange.test.ts
+++ b/exchanges/multipart-fetch/src/multipartFetchExchange.test.ts
@@ -121,7 +121,7 @@ describe('on error', () => {
   beforeEach(() => {
     fetch.mockResolvedValue({
       status: 400,
-      json: jest.fn().mockResolvedValue(response),
+      json: jest.fn().mockResolvedValue({}),
     });
   });
 
@@ -152,6 +152,21 @@ describe('on error', () => {
 
     expect(data).toMatchSnapshot();
   });
+
+  it('ignores the error when a result is available', async () => {
+    fetch.mockResolvedValue({
+      status: 400,
+      json: jest.fn().mockResolvedValue(response),
+    });
+
+    const data = await pipe(
+      fromValue(queryOperation),
+      multipartFetchExchange(exchangeArgs),
+      toPromise
+    );
+
+    expect(data.data).toEqual(response.data);
+  });
 });
 
 describe('on teardown', () => {
diff --git a/packages/core/src/exchanges/fetch.test.ts b/packages/core/src/exchanges/fetch.test.ts
index 2eac8215..6d3394f1 100755
--- a/packages/core/src/exchanges/fetch.test.ts
+++ b/packages/core/src/exchanges/fetch.test.ts
@@ -76,7 +76,7 @@ describe('on error', () => {
   beforeEach(() => {
     fetch.mockResolvedValue({
       status: 400,
-      json: jest.fn().mockResolvedValue(response),
+      json: jest.fn().mockResolvedValue({}),
     });
   });
 
@@ -107,6 +107,21 @@ describe('on error', () => {
 
     expect(data).toMatchSnapshot();
   });
+
+  it('ignores the error when a result is available', async () => {
+    fetch.mockResolvedValue({
+      status: 400,
+      json: jest.fn().mockResolvedValue(response),
+    });
+
+    const data = await pipe(
+      fromValue(queryOperation),
+      fetchExchange(exchangeArgs),
+      toPromise
+    );
+
+    expect(data.data).toEqual(response.data);
+  });
 });
 
 describe('on teardown', () => {