From 1641ab13ae018364ee19e11294c811342b4d6cdf Mon Sep 17 00:00:00 2001 From: Conor Hawes Date: Sat, 4 Sep 2021 15:31:46 -0500 Subject: [PATCH 1/6] Add guard for 204 (don't just rely on content length) --- src/http/http.js | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/http/http.js b/src/http/http.js index 2b5c231..7317352 100644 --- a/src/http/http.js +++ b/src/http/http.js @@ -67,12 +67,12 @@ import { * getUsers() * .then(res => console.log('The users are', res)) * .catch(err => console.log('An error occurred!', err)) - * + * * // Shorthand: pass `url` as first argument: * function getUsers () { * return http('/users', options) * } - * + * */ // Enable shorthand with optional string `url` first argument. @@ -85,9 +85,9 @@ export function parseArguments (...args) { } // Get JSON from response -async function getResponseBody (response) { +async function getResponseBody(response) { // Don't parse empty body - if (response.headers.get('Content-Length') === '0') return null + if (response.headers.get('Content-Length') === '0' || response.status === 204) return null try { return await response.json() } catch (e) { @@ -98,13 +98,12 @@ async function getResponseBody (response) { } async function http (...args) { - - const { + const { before=noop, __mock_response, // used for unit testing ...options } = parseArguments(...args) - + const parsedOptions = await composeMiddleware( before, setDefaults, @@ -117,7 +116,7 @@ async function http (...args) { )(options) const { - onSuccess, + onSuccess, onFailure, camelizeResponse, successDataPath, From d41ce1fb796bbd139e1055352627e481feaf8172 Mon Sep 17 00:00:00 2001 From: Conor Hawes Date: Sat, 4 Sep 2021 15:32:10 -0500 Subject: [PATCH 2/6] Add specs --- __mocks__/isomorphic-fetch.js | 12 ++++++++++-- test/http/http.test.js | 18 ++++++++++++++++-- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/__mocks__/isomorphic-fetch.js b/__mocks__/isomorphic-fetch.js index d480a36..b236e50 100644 --- a/__mocks__/isomorphic-fetch.js +++ b/__mocks__/isomorphic-fetch.js @@ -1,21 +1,29 @@ export const successUrl = '/success' +export const noContentUrl = '/no-content' export const failureUrl = '/failure' export const unauthorizedUrl = '/unauthorized' export const networkErrorUrl = '/network-error' const statuses = { [successUrl]: 200, + [noContentUrl]: 204, [failureUrl]: 400, [unauthorizedUrl]: 401 } export default jest.fn(function (url, options) { + const { headers={} } = options + const body = { ...options, url } + const response = { // Response echoes back passed options headers: { - get: () => {} + get: (header) => { + return headers[header] + }, + ...headers, }, - json: () => Promise.resolve({ ...options, url }), + json: () => Promise.resolve(body), ok: ![failureUrl, unauthorizedUrl].includes(url), status: statuses[url] } diff --git a/test/http/http.test.js b/test/http/http.test.js index e5ba8be..dacafb7 100644 --- a/test/http/http.test.js +++ b/test/http/http.test.js @@ -1,5 +1,5 @@ import Base64 from 'Base64' -import { successUrl, failureUrl } from 'isomorphic-fetch' +import { successUrl, noContentUrl, failureUrl } from 'isomorphic-fetch' import { http } from '../../src' // These tests rely on the mock Fetch() @@ -137,7 +137,7 @@ test('http onFailure hook is not triggered when an error is thrown in onSuccess' expect.assertions(1) const onSuccess = () => { throw new Error('Oops') } const onFailure = jest.fn() - return http(successUrl, { onFailure, onSuccess }).catch(e => { + return http(successUrl, { onFailure, onSuccess }).catch(() => { expect(onFailure).not.toHaveBeenCalled() }) }) @@ -289,6 +289,20 @@ test('http sets basic auth header if `auth` is present', () => { }) }) +test('http returns null when content-length is zero', () => { + return http(successUrl, { headers: { 'Content-Length': '0' }}) + .then((res) => { + expect(res).toBe(null) + }) +}) + +test('http returns null when the status is 204 (no content)', () => { + return http(noContentUrl) + .then((res) => { + expect(res).toBe(null) + }) +}) + /* MOCK STUFF */ // Mock token elements From fd6c433acbf30439e8672d4e0fa7ca8c4b71911e Mon Sep 17 00:00:00 2001 From: Conor Hawes Date: Sat, 4 Sep 2021 15:37:33 -0500 Subject: [PATCH 3/6] Bump patch --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index cbbbad4..53560a6 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@launchpadlab/lp-requests", - "version": "4.1.8", + "version": "4.1.9", "description": "Request helpers", "main": "lib/index.js", "scripts": { From 54b4e4993d7876d105a94a6144de97b2d6b68b87 Mon Sep 17 00:00:00 2001 From: Conor Hawes Date: Sun, 5 Sep 2021 16:25:52 -0500 Subject: [PATCH 4/6] Add option to return text of body if parsing fails --- src/http/http.js | 20 +++++++++++++------- src/http/middleware/set-defaults.js | 5 +++-- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/http/http.js b/src/http/http.js index 7317352..e4c2989 100644 --- a/src/http/http.js +++ b/src/http/http.js @@ -85,15 +85,20 @@ export function parseArguments (...args) { } // Get JSON from response -async function getResponseBody(response) { +async function getResponseBody(response, { parseJsonStrictly }) { // Don't parse empty body if (response.headers.get('Content-Length') === '0' || response.status === 204) return null + let data try { - return await response.json() + data = await response.text() + return JSON.parse(data) } catch (e) { - // eslint-disable-next-line - console.warn('Failed to parse response body: ' + e, response) - return null + if (parseJsonStrictly) { + // eslint-disable-next-line + console.warn('Failed to parse response body: ' + e, response) + return null + } + return data } } @@ -123,13 +128,14 @@ async function http (...args) { failureDataPath, url, fetchOptions, + parseJsonStrictly, } = parsedOptions // responseData is either the body of the response, or an error object. const responseData = await attemptAsync(async () => { // Make request - const response = await fetch(url, fetchOptions) + const response = __mock_response ?? await fetch(url, fetchOptions) // Parse the response - const body = __mock_response || await getResponseBody(response) + const body = await getResponseBody(response, { parseJsonStrictly }) const data = camelizeResponse ? camelizeKeys(body) : body if (!response.ok) { const errors = getDataAtPath(data, failureDataPath) diff --git a/src/http/middleware/set-defaults.js b/src/http/middleware/set-defaults.js index 8a5bac4..32dd739 100644 --- a/src/http/middleware/set-defaults.js +++ b/src/http/middleware/set-defaults.js @@ -3,10 +3,11 @@ import { identity } from '../../utils' const DEFAULTS = { credentials: 'same-origin', mode: 'same-origin', - onSuccess: identity, + onSuccess: identity, onFailure: identity, camelizeResponse: true, failureDataPath: 'errors', + parseJsonStrictly: true, } const DEFAULT_HEADERS = { @@ -25,4 +26,4 @@ function setDefaults ({ headers={}, overrideHeaders=false, ...rest }) { } } -export default setDefaults \ No newline at end of file +export default setDefaults From 7ee9316b9707e31ed07b1d2f9a6342a5cbce119f Mon Sep 17 00:00:00 2001 From: Conor Hawes Date: Sun, 5 Sep 2021 16:26:05 -0500 Subject: [PATCH 5/6] Add specs --- __mocks__/isomorphic-fetch.js | 40 +++++++++++++++++++----------- test/http/http.test.js | 46 +++++++++++++++++++++++------------ 2 files changed, 57 insertions(+), 29 deletions(-) diff --git a/__mocks__/isomorphic-fetch.js b/__mocks__/isomorphic-fetch.js index b236e50..00425d2 100644 --- a/__mocks__/isomorphic-fetch.js +++ b/__mocks__/isomorphic-fetch.js @@ -11,22 +11,34 @@ const statuses = { [unauthorizedUrl]: 401 } -export default jest.fn(function (url, options) { - const { headers={} } = options - const body = { ...options, url } +export class MockResponse { + #text - const response = { - // Response echoes back passed options - headers: { - get: (header) => { - return headers[header] - }, - ...headers, - }, - json: () => Promise.resolve(body), - ok: ![failureUrl, unauthorizedUrl].includes(url), - status: statuses[url] + constructor(url, options, body = null) { + const { headers={} } = options + this.headers = { + get: (header) => headers[header], + ...headers + } + this.body = body ?? { ...options, url } + this.ok = ![failureUrl, unauthorizedUrl].includes(url) + this.status = statuses[url] + this._config = options + this.#text = typeof body === 'string' ? body : JSON.stringify(this.body) } + + json() { + return Promise.resolve(this.body) + } + + text() { + return Promise.resolve(this.#text) + } +} + +export default jest.fn(function fetch(url, options) { + const response = new MockResponse(url, options) + // Simulate server response return new Promise((resolve, reject) => { setTimeout( diff --git a/test/http/http.test.js b/test/http/http.test.js index dacafb7..72218f3 100644 --- a/test/http/http.test.js +++ b/test/http/http.test.js @@ -1,5 +1,5 @@ import Base64 from 'Base64' -import { successUrl, noContentUrl, failureUrl } from 'isomorphic-fetch' +import { successUrl, noContentUrl, failureUrl, MockResponse } from 'isomorphic-fetch' import { http } from '../../src' // These tests rely on the mock Fetch() @@ -81,7 +81,7 @@ test('http modifies fetch configuration using `before` hook', () => { test('http modifies overall configuration using `before` hook', () => { const before = () => ({ successDataPath: 'foo' }) - return http(successUrl, { before, __mock_response: { foo: 'bar' } }).then((res) => { + return httpWithMock(successUrl, { before }, { foo: 'bar' }).then((res) => { expect(res).toEqual('bar') }) }) @@ -180,12 +180,11 @@ test('http pulls data from response using successDataPath', () => { test('http failureDataPath defaults to "errors"', () => { expect.assertions(1) const ERRORS = { 'someValue': 'there was an error' } - return http(failureUrl, { + return httpWithMock(failureUrl, { method: 'POST', - __mock_response: { + }, { errors: ERRORS, - } - }).catch((err) => { + }).catch((err) => { expect(err.errors).toEqual(ERRORS) }) }) @@ -231,23 +230,21 @@ test('http does not decamelizes query if decamelizeQuery is false', () => { }) test('http camelizes json response by default', () => { - return http(successUrl, { + return httpWithMock(successUrl, { method: 'POST', - __mock_response: { + }, { camelized_key: 'a camelized key' - }, - }).then((res) => { + }).then((res) => { expect(res).toHaveProperty('camelizedKey') }) }) -test('http does not camelizes json response if camelize passed as false', () => { - return http(successUrl, { +test('http does not camelize json response if camelize passed as false', () => { + return httpWithMock(successUrl, { camelizeResponse: false, - __mock_response: { + }, { Capitalized_key: 'a weirdly cased key' - } - }).then((res) => { + }).then((res) => { expect(res).toHaveProperty('Capitalized_key') }) }) @@ -303,8 +300,27 @@ test('http returns null when the status is 204 (no content)', () => { }) }) +test('http returns null when json parsing fails by default', () => { + return httpWithMock(successUrl, {}, "123AZY") + .then((res) => { + expect(res).toBe(null) + }) +}) + +test('http returns text of body when json parsing fails and parseJsonStrictly is `false`', () => { + return httpWithMock(successUrl, { parseJsonStrictly: false }, "123AZY") + .then((res) => { + expect(res).toBe('123AZY') + }) +}) + /* MOCK STUFF */ +async function httpWithMock(url, options, responseBody) { + const mockResponse = new MockResponse(url, options, responseBody) + return http(url, { ...options, __mock_response: mockResponse }) +} + // Mock token elements const createTokenTag = (name, content) => { const tag = document.createElement('meta') From 886cf297dca467945d2fc9679fdb894edf03d17c Mon Sep 17 00:00:00 2001 From: Conor Hawes Date: Mon, 6 Sep 2021 09:19:09 -0500 Subject: [PATCH 6/6] Update docs and version --- docs.md | 1 + package.json | 2 +- src/http/http.js | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/docs.md b/docs.md index d6ce49e..6c887e7 100644 --- a/docs.md +++ b/docs.md @@ -171,6 +171,7 @@ In addition to the normal Fetch API settings, the config object may also contain - `camelizeResponse`: A boolean flag indicating whether or not to camelize the response keys (default=`true`). The helper function that does this is also exported from this library as `camelizeKeys`. - `decamelizeBody`: A boolean flag indicating whether or not to decamelize the body keys (default=`true`). The helper function that does this is also exported from this library as `decamelizeKeys`. - `decamelizeQuery`: A boolean flag indicating whether or not to decamelize the query string keys (default=`true`). +- `parseJsonStrictly`: A boolean flag indicating whether or not to return the text of the response body if JSON parsing fails (default=`true`). If set to `true` and invalid JSON is received in the response, then `null` will be returned instead. - `auth`: An object with the following keys `{ username, password }`. If present, `http` will use [basic auth][28], adding the header `"Authorization": "Basic "` to the request, where `` is a base64 encoded string of `username:password`. ### Parameters diff --git a/package.json b/package.json index 53560a6..1bece6e 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@launchpadlab/lp-requests", - "version": "4.1.9", + "version": "4.2.0", "description": "Request helpers", "main": "lib/index.js", "scripts": { diff --git a/src/http/http.js b/src/http/http.js index e4c2989..2d625f8 100644 --- a/src/http/http.js +++ b/src/http/http.js @@ -47,6 +47,7 @@ import { * - `camelizeResponse`: A boolean flag indicating whether or not to camelize the response keys (default=`true`). The helper function that does this is also exported from this library as `camelizeKeys`. * - `decamelizeBody`: A boolean flag indicating whether or not to decamelize the body keys (default=`true`). The helper function that does this is also exported from this library as `decamelizeKeys`. * - `decamelizeQuery`: A boolean flag indicating whether or not to decamelize the query string keys (default=`true`). + * - `parseJsonStrictly`: A boolean flag indicating whether or not to return the text of the response body if JSON parsing fails (default=`true`). If set to `true` and invalid JSON is received in the response, then `null` will be returned instead. * - `auth`: An object with the following keys `{ username, password }`. If present, `http` will use [basic auth](https://en.wikipedia.org/wiki/Basic_access_authentication#Client_side), adding the header `"Authorization": "Basic "` to the request, where `` is a base64 encoded string of `username:password`. * * @name http