From e06271ea7b626ff8bf696647cb8a193bb3eb27da Mon Sep 17 00:00:00 2001 From: jdecroock Date: Fri, 13 May 2022 11:11:37 +0200 Subject: [PATCH 1/5] handle text responses --- .changeset/moody-brooms-refuse.md | 5 +++++ packages/core/src/internal/fetchSource.ts | 13 +++++++++---- 2 files changed, 14 insertions(+), 4 deletions(-) create mode 100644 .changeset/moody-brooms-refuse.md diff --git a/.changeset/moody-brooms-refuse.md b/.changeset/moody-brooms-refuse.md new file mode 100644 index 0000000000..436c4d3bff --- /dev/null +++ b/.changeset/moody-brooms-refuse.md @@ -0,0 +1,5 @@ +--- +'@urql/core': patch +--- + +Handle text responses correctly diff --git a/packages/core/src/internal/fetchSource.ts b/packages/core/src/internal/fetchSource.ts index 9b956b2d0a..f8d0c5ffd1 100644 --- a/packages/core/src/internal/fetchSource.ts +++ b/packages/core/src/internal/fetchSource.ts @@ -44,10 +44,15 @@ export const makeFetchSource = ( const contentType = (response.headers && response.headers.get('Content-Type')) || ''; if (!/multipart\/mixed/i.test(contentType)) { - return response.json().then(payload => { - const result = makeResult(operation, payload, response); - hasResults = true; - onResult(result); + return response.text().then(text => { + try { + const payload = JSON.parse(text); + hasResults = true; + const result = makeResult(operation, payload, response); + onResult(result); + } catch (e) { + throw new Error(text); + } }); } From 31b91271022ea919601443a2d85c73decafb47ed Mon Sep 17 00:00:00 2001 From: jdecroock Date: Fri, 13 May 2022 11:16:05 +0200 Subject: [PATCH 2/5] update tests --- packages/core/src/internal/fetchSource.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/core/src/internal/fetchSource.test.ts b/packages/core/src/internal/fetchSource.test.ts index a8e97fd7ce..a5dbef46da 100644 --- a/packages/core/src/internal/fetchSource.test.ts +++ b/packages/core/src/internal/fetchSource.test.ts @@ -28,20 +28,20 @@ afterAll(() => { (global as any).AbortController = undefined; }); -const response = { +const response = JSON.stringify({ status: 200, data: { data: { user: 1200, }, }, -}; +}); describe('on success', () => { beforeEach(() => { fetch.mockResolvedValue({ status: 200, - json: jest.fn().mockResolvedValue(response), + text: jest.fn().mockResolvedValue(response), }); }); @@ -63,7 +63,7 @@ describe('on success', () => { const fetchOptions = {}; const fetcher = jest.fn().mockResolvedValue({ status: 200, - json: jest.fn().mockResolvedValue(response), + text: jest.fn().mockResolvedValue(response), }); const data = await pipe( @@ -91,7 +91,7 @@ describe('on error', () => { beforeEach(() => { fetch.mockResolvedValue({ status: 400, - json: jest.fn().mockResolvedValue({}), + text: jest.fn().mockResolvedValue({}), }); }); From 299b413ca87e3a40a735cdfd5cc6458be44db1c7 Mon Sep 17 00:00:00 2001 From: jdecroock Date: Fri, 13 May 2022 11:28:38 +0200 Subject: [PATCH 3/5] fix tests --- exchanges/execute/src/execute.test.ts | 4 +- .../multipartFetchExchange.test.ts.snap | 4 +- .../src/multipartFetchExchange.test.ts | 12 ++-- .../src/persistedFetchExchange.test.ts | 62 +++++++++---------- .../__snapshots__/fetch.test.ts.snap | 4 +- packages/core/src/exchanges/fetch.test.ts | 12 ++-- .../__snapshots__/fetchSource.test.ts.snap | 6 +- packages/core/src/internal/fetchSource.ts | 7 ++- 8 files changed, 59 insertions(+), 52 deletions(-) diff --git a/exchanges/execute/src/execute.test.ts b/exchanges/execute/src/execute.test.ts index ff83cd7054..a80d35e7c7 100644 --- a/exchanges/execute/src/execute.test.ts +++ b/exchanges/execute/src/execute.test.ts @@ -167,7 +167,9 @@ describe('on operation', () => { fetchMock.mockResolvedValue({ status: 200, - json: jest.fn().mockResolvedValue({ data: mockHttpResponseData }), + text: jest + .fn() + .mockResolvedValue(JSON.stringify({ data: mockHttpResponseData })), }); const responseFromFetchExchange = await pipe( diff --git a/exchanges/multipart-fetch/src/__snapshots__/multipartFetchExchange.test.ts.snap b/exchanges/multipart-fetch/src/__snapshots__/multipartFetchExchange.test.ts.snap index 5c52fb12f2..b0dad0110d 100644 --- a/exchanges/multipart-fetch/src/__snapshots__/multipartFetchExchange.test.ts.snap +++ b/exchanges/multipart-fetch/src/__snapshots__/multipartFetchExchange.test.ts.snap @@ -3,7 +3,7 @@ exports[`on error returns error data 1`] = ` Object { "data": undefined, - "error": [CombinedError: [Network] ], + "error": [CombinedError: [Network] {}], "extensions": undefined, "operation": Object { "context": Object { @@ -142,7 +142,7 @@ query getUser($name: String) { user(name: $name) { id firstName lastName } }", exports[`on error returns error data with status 400 and manual redirect mode 1`] = ` Object { "data": undefined, - "error": [CombinedError: [Network] ], + "error": [CombinedError: [Network] {}], "extensions": undefined, "operation": Object { "context": Object { diff --git a/exchanges/multipart-fetch/src/multipartFetchExchange.test.ts b/exchanges/multipart-fetch/src/multipartFetchExchange.test.ts index 4895c5d7d4..d3c29d8de5 100644 --- a/exchanges/multipart-fetch/src/multipartFetchExchange.test.ts +++ b/exchanges/multipart-fetch/src/multipartFetchExchange.test.ts @@ -31,14 +31,14 @@ afterAll(() => { (global as any).AbortController = undefined; }); -const response = { +const response = JSON.stringify({ status: 200, data: { data: { user: 1200, }, }, -}; +}); const exchangeArgs = { forward: () => empty as Source, @@ -50,7 +50,7 @@ describe('on success', () => { beforeEach(() => { fetch.mockResolvedValue({ status: 200, - json: jest.fn().mockResolvedValue(response), + text: jest.fn().mockResolvedValue(response), }); }); @@ -121,7 +121,7 @@ describe('on error', () => { beforeEach(() => { fetch.mockResolvedValue({ status: 400, - json: jest.fn().mockResolvedValue({}), + text: jest.fn().mockResolvedValue('{}'), }); }); @@ -156,7 +156,7 @@ describe('on error', () => { it('ignores the error when a result is available', async () => { fetch.mockResolvedValue({ status: 400, - json: jest.fn().mockResolvedValue(response), + text: jest.fn().mockResolvedValue(response), }); const data = await pipe( @@ -165,7 +165,7 @@ describe('on error', () => { toPromise ); - expect(data.data).toEqual(response.data); + expect(data.data).toEqual(JSON.parse(response).data); }); }); diff --git a/exchanges/persisted-fetch/src/persistedFetchExchange.test.ts b/exchanges/persisted-fetch/src/persistedFetchExchange.test.ts index abe4f9bc1d..62bb2cb590 100644 --- a/exchanges/persisted-fetch/src/persistedFetchExchange.test.ts +++ b/exchanges/persisted-fetch/src/persistedFetchExchange.test.ts @@ -28,14 +28,14 @@ afterEach(() => { }); it('accepts successful persisted query responses', async () => { - const expected = { + const expected = JSON.stringify({ data: { test: true, }, - }; + }); fetch.mockResolvedValueOnce({ - json: () => Promise.resolve(expected), + text: () => Promise.resolve(expected), }); const actual = await pipe( @@ -50,22 +50,22 @@ it('accepts successful persisted query responses', async () => { }); it('supports cache-miss persisted query errors', async () => { - const expectedMiss = { + const expectedMiss = JSON.stringify({ errors: [{ message: 'PersistedQueryNotFound' }], - }; + }); - const expectedRetry = { + const expectedRetry = JSON.stringify({ data: { test: true, }, - }; + }); fetch .mockResolvedValueOnce({ - json: () => Promise.resolve(expectedMiss), + text: () => Promise.resolve(expectedMiss), }) .mockResolvedValueOnce({ - json: () => Promise.resolve(expectedRetry), + text: () => Promise.resolve(expectedRetry), }); const actual = await pipe( @@ -81,22 +81,22 @@ it('supports cache-miss persisted query errors', async () => { }); it('supports GET exclusively for persisted queries', async () => { - const expectedMiss = { + const expectedMiss = JSON.stringify({ errors: [{ message: 'PersistedQueryNotFound' }], - }; + }); - const expectedRetry = { + const expectedRetry = JSON.stringify({ data: { test: true, }, - }; + }); fetch .mockResolvedValueOnce({ - json: () => Promise.resolve(expectedMiss), + text: () => Promise.resolve(expectedMiss), }) .mockResolvedValueOnce({ - json: () => Promise.resolve(expectedRetry), + text: () => Promise.resolve(expectedRetry), }); const actual = await pipe( @@ -114,25 +114,25 @@ it('supports GET exclusively for persisted queries', async () => { }); it('supports unsupported persisted query errors', async () => { - const expectedMiss = { + const expectedMiss = JSON.stringify({ errors: [{ message: 'PersistedQueryNotSupported' }], - }; + }); - const expectedRetry = { + const expectedRetry = JSON.stringify({ data: { test: true, }, - }; + }); fetch .mockResolvedValueOnce({ - json: () => Promise.resolve(expectedMiss), + text: () => Promise.resolve(expectedMiss), }) .mockResolvedValueOnce({ - json: () => Promise.resolve(expectedRetry), + text: () => Promise.resolve(expectedRetry), }) .mockResolvedValueOnce({ - json: () => Promise.resolve(expectedRetry), + text: () => Promise.resolve(expectedRetry), }); const actual = await pipe( @@ -148,14 +148,14 @@ it('supports unsupported persisted query errors', async () => { }); it('correctly generates an SHA256 hash', async () => { - const expected = { + const expected = JSON.stringify({ data: { test: true, }, - }; + }); fetch.mockResolvedValue({ - json: () => Promise.resolve(expected), + text: () => Promise.resolve(expected), }); const queryHash = await hash(print(queryOperation.query)); @@ -185,14 +185,14 @@ it('correctly generates an SHA256 hash', async () => { }); it('supports a custom hash function', async () => { - const expected = { + const expected = JSON.stringify({ data: { test: true, }, - }; + }); fetch.mockResolvedValueOnce({ - json: () => Promise.resolve(expected), + text: () => Promise.resolve(expected), }); const hashFn = jest.fn((_input: string, _doc: DocumentNode) => { @@ -228,14 +228,14 @@ it('supports a custom hash function', async () => { }); it('falls back to a non-persisted query if the hash is falsy', async () => { - const expected = { + const expected = JSON.stringify({ data: { test: true, }, - }; + }); fetch.mockResolvedValueOnce({ - json: () => Promise.resolve(expected), + text: () => Promise.resolve(expected), }); const hashFn = jest.fn(() => Promise.resolve('')); diff --git a/packages/core/src/exchanges/__snapshots__/fetch.test.ts.snap b/packages/core/src/exchanges/__snapshots__/fetch.test.ts.snap index f891b3a816..510578a027 100644 --- a/packages/core/src/exchanges/__snapshots__/fetch.test.ts.snap +++ b/packages/core/src/exchanges/__snapshots__/fetch.test.ts.snap @@ -3,7 +3,7 @@ exports[`on error returns error data 1`] = ` Object { "data": undefined, - "error": [CombinedError: [Network] ], + "error": [CombinedError: [Network] {}], "extensions": undefined, "operation": Object { "context": Object { @@ -142,7 +142,7 @@ query getUser($name: String) { user(name: $name) { id firstName lastName } }", exports[`on error returns error data with status 400 and manual redirect mode 1`] = ` Object { "data": undefined, - "error": [CombinedError: [Network] ], + "error": [CombinedError: [Network] {}], "extensions": undefined, "operation": Object { "context": Object { diff --git a/packages/core/src/exchanges/fetch.test.ts b/packages/core/src/exchanges/fetch.test.ts index 5320af06d9..5eb5a40c82 100755 --- a/packages/core/src/exchanges/fetch.test.ts +++ b/packages/core/src/exchanges/fetch.test.ts @@ -28,14 +28,14 @@ afterAll(() => { (global as any).AbortController = undefined; }); -const response = { +const response = JSON.stringify({ status: 200, data: { data: { user: 1200, }, }, -}; +}); const exchangeArgs = { dispatchDebug: jest.fn(), @@ -51,7 +51,7 @@ describe('on success', () => { beforeEach(() => { fetch.mockResolvedValue({ status: 200, - json: jest.fn().mockResolvedValue(response), + text: jest.fn().mockResolvedValue(response), }); }); @@ -80,7 +80,7 @@ describe('on error', () => { beforeEach(() => { fetch.mockResolvedValue({ status: 400, - json: jest.fn().mockResolvedValue({}), + text: jest.fn().mockResolvedValue(JSON.stringify({})), }); }); @@ -115,7 +115,7 @@ describe('on error', () => { it('ignores the error when a result is available', async () => { fetch.mockResolvedValue({ status: 400, - json: jest.fn().mockResolvedValue(response), + text: jest.fn().mockResolvedValue(response), }); const data = await pipe( @@ -124,7 +124,7 @@ describe('on error', () => { toPromise ); - expect(data.data).toEqual(response.data); + expect(data.data).toEqual(JSON.parse(response).data); }); }); diff --git a/packages/core/src/internal/__snapshots__/fetchSource.test.ts.snap b/packages/core/src/internal/__snapshots__/fetchSource.test.ts.snap index 2620471a00..22810562a5 100644 --- a/packages/core/src/internal/__snapshots__/fetchSource.test.ts.snap +++ b/packages/core/src/internal/__snapshots__/fetchSource.test.ts.snap @@ -3,7 +3,7 @@ exports[`on error ignores the error when a result is available 1`] = ` Object { "data": undefined, - "error": [CombinedError: [Network] ], + "error": [CombinedError: [Network] [object Object]], "extensions": undefined, "operation": Object { "context": Object { @@ -142,7 +142,7 @@ query getUser($name: String) { user(name: $name) { id firstName lastName } }", exports[`on error returns error data 1`] = ` Object { "data": undefined, - "error": [CombinedError: [Network] ], + "error": [CombinedError: [Network] [object Object]], "extensions": undefined, "operation": Object { "context": Object { @@ -281,7 +281,7 @@ query getUser($name: String) { user(name: $name) { id firstName lastName } }", exports[`on error returns error data with status 400 and manual redirect mode 1`] = ` Object { "data": undefined, - "error": [CombinedError: [Network] ], + "error": [CombinedError: [Network] [object Object]], "extensions": undefined, "operation": Object { "context": Object { diff --git a/packages/core/src/internal/fetchSource.ts b/packages/core/src/internal/fetchSource.ts index f8d0c5ffd1..3917818abd 100644 --- a/packages/core/src/internal/fetchSource.ts +++ b/packages/core/src/internal/fetchSource.ts @@ -51,7 +51,12 @@ export const makeFetchSource = ( const result = makeResult(operation, payload, response); onResult(result); } catch (e) { - throw new Error(text); + const result = makeErrorResult( + operation, + new Error(text), + response + ); + onResult(result); } }); } From 40a33202b4a398cc53ba5f5fa1cb9fad8b0319aa Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Tue, 24 May 2022 16:53:12 +0100 Subject: [PATCH 4/5] Replace error fallback with text/* special content handling The outer `catch` was still responsible for handling errors, so we can avoid duplication here and still use `statusText`. We should instead add a separate case for `text/*` contents. Those should never really be returned by endpoints, but if they are we can return them as it was in this branch before. --- .changeset/moody-brooms-refuse.md | 2 +- .../__snapshots__/fetch.test.ts.snap | 4 +-- .../__snapshots__/fetchSource.test.ts.snap | 6 ++--- .../core/src/internal/fetchSource.test.ts | 25 +++++++++++++++++- packages/core/src/internal/fetchSource.ts | 26 ++++++++----------- 5 files changed, 41 insertions(+), 22 deletions(-) diff --git a/.changeset/moody-brooms-refuse.md b/.changeset/moody-brooms-refuse.md index 436c4d3bff..636a5dd7fa 100644 --- a/.changeset/moody-brooms-refuse.md +++ b/.changeset/moody-brooms-refuse.md @@ -2,4 +2,4 @@ '@urql/core': patch --- -Handle text responses correctly +Passthrough responses with content type of `text/*` as error messages. diff --git a/packages/core/src/exchanges/__snapshots__/fetch.test.ts.snap b/packages/core/src/exchanges/__snapshots__/fetch.test.ts.snap index 510578a027..5ee9e4f64f 100644 --- a/packages/core/src/exchanges/__snapshots__/fetch.test.ts.snap +++ b/packages/core/src/exchanges/__snapshots__/fetch.test.ts.snap @@ -3,7 +3,7 @@ exports[`on error returns error data 1`] = ` Object { "data": undefined, - "error": [CombinedError: [Network] {}], + "error": [CombinedError: [Network] No Content], "extensions": undefined, "operation": Object { "context": Object { @@ -142,7 +142,7 @@ query getUser($name: String) { user(name: $name) { id firstName lastName } }", exports[`on error returns error data with status 400 and manual redirect mode 1`] = ` Object { "data": undefined, - "error": [CombinedError: [Network] {}], + "error": [CombinedError: [Network] No Content], "extensions": undefined, "operation": Object { "context": Object { diff --git a/packages/core/src/internal/__snapshots__/fetchSource.test.ts.snap b/packages/core/src/internal/__snapshots__/fetchSource.test.ts.snap index 22810562a5..c552c37da3 100644 --- a/packages/core/src/internal/__snapshots__/fetchSource.test.ts.snap +++ b/packages/core/src/internal/__snapshots__/fetchSource.test.ts.snap @@ -3,7 +3,7 @@ exports[`on error ignores the error when a result is available 1`] = ` Object { "data": undefined, - "error": [CombinedError: [Network] [object Object]], + "error": [CombinedError: [Network] Forbidden], "extensions": undefined, "operation": Object { "context": Object { @@ -142,7 +142,7 @@ query getUser($name: String) { user(name: $name) { id firstName lastName } }", exports[`on error returns error data 1`] = ` Object { "data": undefined, - "error": [CombinedError: [Network] [object Object]], + "error": [CombinedError: [Network] Forbidden], "extensions": undefined, "operation": Object { "context": Object { @@ -281,7 +281,7 @@ query getUser($name: String) { user(name: $name) { id firstName lastName } }", exports[`on error returns error data with status 400 and manual redirect mode 1`] = ` Object { "data": undefined, - "error": [CombinedError: [Network] [object Object]], + "error": [CombinedError: [Network] Forbidden], "extensions": undefined, "operation": Object { "context": Object { diff --git a/packages/core/src/internal/fetchSource.test.ts b/packages/core/src/internal/fetchSource.test.ts index a5dbef46da..c2a55f6beb 100644 --- a/packages/core/src/internal/fetchSource.test.ts +++ b/packages/core/src/internal/fetchSource.test.ts @@ -91,7 +91,8 @@ describe('on error', () => { beforeEach(() => { fetch.mockResolvedValue({ status: 400, - text: jest.fn().mockResolvedValue({}), + statusText: 'Forbidden', + text: jest.fn().mockResolvedValue('{}'), }); }); @@ -126,6 +127,28 @@ describe('on error', () => { }); }); +describe('on unexpected plain text responses', () => { + beforeEach(() => { + fetch.mockResolvedValue({ + status: 200, + headers: new Map([['Content-Type', 'text/plain']]), + text: jest.fn().mockResolvedValue('Some Error Message'), + }); + }); + + it('returns error data', async () => { + const fetchOptions = {}; + const result = await pipe( + makeFetchSource(queryOperation, 'https://test.com/graphql', fetchOptions), + toPromise + ); + + expect(result.error).toMatchObject({ + message: '[Network] Some Error Message', + }); + }); +}); + describe('on teardown', () => { it('does not start the outgoing request on immediate teardowns', () => { fetch.mockRejectedValue(abortError); diff --git a/packages/core/src/internal/fetchSource.ts b/packages/core/src/internal/fetchSource.ts index 3917818abd..aaeb1a7d75 100644 --- a/packages/core/src/internal/fetchSource.ts +++ b/packages/core/src/internal/fetchSource.ts @@ -43,21 +43,13 @@ export const makeFetchSource = ( // NOTE: Guarding against fetch polyfills here const contentType = (response.headers && response.headers.get('Content-Type')) || ''; - if (!/multipart\/mixed/i.test(contentType)) { + if (/text\//i.test(contentType)) { return response.text().then(text => { - try { - const payload = JSON.parse(text); - hasResults = true; - const result = makeResult(operation, payload, response); - onResult(result); - } catch (e) { - const result = makeErrorResult( - operation, - new Error(text), - response - ); - onResult(result); - } + onResult(makeErrorResult(operation, new Error(text), response)); + }); + } else if (!/multipart\/mixed/i.test(contentType)) { + return response.text().then(payload => { + onResult(makeResult(operation, JSON.parse(payload), response)); }); } @@ -170,7 +162,11 @@ export const makeFetchSource = ( const result = makeErrorResult( operation, - statusNotOk ? new Error(response.statusText) : error, + statusNotOk + ? response.statusText + ? new Error(response.statusText) + : error + : error, response ); From 11d472b3d6b59fd3f71845261cf33a2c266e5c4f Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Tue, 24 May 2022 17:23:42 +0100 Subject: [PATCH 5/5] Update multipart snapshots --- .../src/__snapshots__/multipartFetchExchange.test.ts.snap | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/exchanges/multipart-fetch/src/__snapshots__/multipartFetchExchange.test.ts.snap b/exchanges/multipart-fetch/src/__snapshots__/multipartFetchExchange.test.ts.snap index b0dad0110d..11df69bb55 100644 --- a/exchanges/multipart-fetch/src/__snapshots__/multipartFetchExchange.test.ts.snap +++ b/exchanges/multipart-fetch/src/__snapshots__/multipartFetchExchange.test.ts.snap @@ -3,7 +3,7 @@ exports[`on error returns error data 1`] = ` Object { "data": undefined, - "error": [CombinedError: [Network] {}], + "error": [CombinedError: [Network] No Content], "extensions": undefined, "operation": Object { "context": Object { @@ -142,7 +142,7 @@ query getUser($name: String) { user(name: $name) { id firstName lastName } }", exports[`on error returns error data with status 400 and manual redirect mode 1`] = ` Object { "data": undefined, - "error": [CombinedError: [Network] {}], + "error": [CombinedError: [Network] No Content], "extensions": undefined, "operation": Object { "context": Object {