From 1ecf69f750fb69914af226713ff0c88d8ec2110c Mon Sep 17 00:00:00 2001 From: Jmeas Date: Thu, 12 Apr 2018 21:33:44 -0700 Subject: [PATCH 1/8] Begin working on bug fix for cache mismatch --- README.md | 5 +- src/fetch.js | 91 ++++++++++---- test/index.test.js | 18 ++- test/responses.js | 7 ++ test/same-component.test.js | 244 ++++++++++++++++++++++++++++++++++++ 5 files changed, 341 insertions(+), 24 deletions(-) create mode 100644 test/same-component.test.js diff --git a/README.md b/README.md index 3ef7359..697ecb8 100644 --- a/README.md +++ b/README.md @@ -225,8 +225,9 @@ There are three common use cases for the `doFetch` prop: is passed as `true`. `doFetch` accepts one argument: `options`. Any of the `fetch()` options, such as `url`, `method`, and -`body` are valid `options`. This allows you to customize the request from within the component based -on the component's state. +`body` are valid `options`. You may also specify a new `requestKey` if you are manually generating your +own keys. This method allows you to customize the request from within the component based on the +component's state. ##### `lazy` diff --git a/src/fetch.js b/src/fetch.js index b7eb7e3..128e8aa 100644 --- a/src/fetch.js +++ b/src/fetch.js @@ -95,22 +95,25 @@ export class Fetch extends React.Component { } } - componentWillReceiveProps(nextProps) { + componentDidUpdate(prevProps) { const currentRequestKey = this.props.requestKey || getRequestKey({ ...this.props, method: this.props.method.toUpperCase() }); - const nextRequestKey = - nextProps.requestKey || + // + const prevRequestKey = + prevProps.requestKey || getRequestKey({ - ...nextProps, - method: this.props.method.toUpperCase() + ...prevProps, + method: prevProps.method.toUpperCase() }); - if (currentRequestKey !== nextRequestKey && !this.isLazy(nextProps)) { - this.fetchData(nextProps); + if (currentRequestKey !== prevRequestKey && !this.isLazy(prevProps)) { + this.fetchData({ + requestKey: currentRequestKey + }); } } @@ -145,10 +148,60 @@ export class Fetch extends React.Component { }); }; + // When a subsequent request is made, it is important that the correct + // request key is used. This method computes the right key based on the + // options and props. + getRequestKey = options => { + // A request key in the options gets top priority + if (options && options.requestKey) { + return options.requestKey; + } + + // Otherwise, if we have no request key, but we do have options, then we + // recompute the request key based on these options. + // Note that if the URL, body, or method have not changed, then the request + // key should match the previous request key if it was computed. + // If you passed in a custom request key as a prop, then you will also + // need to pass in a custom key when you call `doFetch()`! + else if (options) { + const { url, method, body } = Object.assign({}, this.props, options); + return getRequestKey({ + url, + body, + method: method.toUpperCase() + }); + } + + // Next in line is the the request key from props. + else if (this.props.requestKey) { + return this.props.requestKey; + } + + // Lastly, we compute the request key from the props. + else { + const { url, method, body } = this.props; + + return getRequestKey({ + url, + body, + method: method.toUpperCase() + }); + } + }; + + // Heads up: accessing `this.props` within `fetchData` + // is unsafe! This is because we call `fetchData` from + // within `componentWillReceiveProps`! fetchData = (options, ignoreCache) => { + // These are the things that we do not allow a user to configure in + // `options` when calling `doFetch()`. Perhaps we should, however. const { requestName, dedupe, beforeFetch } = this.props; - this.cancelExistingRequest('New fetch initiated'); + this.cancelExistingRequest('New fetch initiated', this.props); + + const requestKey = this.getRequestKey(this.props, options); + + const requestOptions = Object.assign({}, this.props, options); const { url, @@ -165,16 +218,7 @@ export class Fetch extends React.Component { integrity, keepalive, signal - } = Object.assign({}, this.props, options); - - // We need to compute a new key, just in case a new value was passed in `doFetch`. - const requestKey = - this.props.requestKey || - getRequestKey({ - url, - method: method.toUpperCase(), - body - }); + } = requestOptions; const uppercaseMethod = method.toUpperCase(); const shouldCacheResponse = this.shouldCacheResponse(); @@ -238,8 +282,12 @@ export class Fetch extends React.Component { } this.setState({ - fetching: true, - requestKey + // Note: when data is returned from the cache, the calls to `onResponseReceived` + // will call `setState` to update the `requestKey`. This call to update `requestKey` + // may be duplicated in those situations, but that should be OK. It is necessary + // to include this here to account for "network-only" fetch policies. + requestKey, + fetching: true }); const hittingNetwork = !isRequestInFlight(requestKey) || !dedupe; @@ -334,7 +382,8 @@ export class Fetch extends React.Component { data, error, response, - fetching: stillFetching + fetching: stillFetching, + requestKey }, () => this.props.onResponse(error, response) ); diff --git a/test/index.test.js b/test/index.test.js index f3954a8..6bb8520 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -7,7 +7,7 @@ import { getRequestKey, clearResponseCache } from '../src'; -import { successfulResponse, jsonResponse } from './responses'; +import { successfulResponse, jsonResponse, jsonResponse2 } from './responses'; import { setTimeout } from 'timers'; function hangingPromise() { @@ -70,6 +70,22 @@ fetchMock.post( }) ); +fetchMock.get( + '/test/succeeds/json-one', + () => + new Promise(resolve => { + resolve(jsonResponse()); + }) +); + +fetchMock.get( + '/test/succeeds/json-two', + () => + new Promise(resolve => { + resolve(jsonResponse2()); + }) +); + // Some time for the mock fetches to resolve const networkTimeout = 10; diff --git a/test/responses.js b/test/responses.js index fe3b840..723b08e 100644 --- a/test/responses.js +++ b/test/responses.js @@ -11,3 +11,10 @@ export function jsonResponse() { statusText: 'OK' }); } + +export function jsonResponse2() { + return new Response('{"authors": [22, 13]}', { + status: 200, + statusText: 'OK' + }); +} diff --git a/test/same-component.test.js b/test/same-component.test.js new file mode 100644 index 0000000..27db924 --- /dev/null +++ b/test/same-component.test.js @@ -0,0 +1,244 @@ +import React from 'react'; +import fetchMock from 'fetch-mock'; +import { mount } from 'enzyme'; +import { Fetch, clearRequestCache, clearResponseCache } from '../src'; +import { jsonResponse, jsonResponse2 } from './responses'; +import { setTimeout } from 'timers'; + +fetchMock.get( + '/test/succeeds/json-one', + () => + new Promise(resolve => { + resolve(jsonResponse()); + }) +); + +fetchMock.get( + '/test/succeeds/json-two', + () => + new Promise(resolve => { + resolve(jsonResponse2()); + }) +); + +// Some time for the mock fetches to resolve +const networkTimeout = 10; + +beforeEach(() => { + clearRequestCache(); + clearResponseCache(); +}); + +describe('same-component subsequent requests with caching (gh-151)', () => { + // Issue 151 describes the 3 situations when requests can be made. "Prop changes" + // refers to situation 2. + describe('prop changes', () => { + test('it uses a directly-updated request key on subsequent renders', done => { + // expect.assertions(8); + const onResponseMock = jest.fn(); + const beforeFetchMock = jest.fn(); + const afterFetchMock = jest.fn(); + + let secondRun = false; + let secondRenderCount = 0; + + const wrapper = mount( + + {options => { + if (secondRun) { + // Increment our render count. This allows us to + // test for each of the individual renders involved + // with changing the prop. + secondRenderCount++; + + // This first render is interesting: we basically only have a + // new URL set, but the request has not yet begun. The reason + // for this is because we do the fetch in `componentDidUpdate`. + if (secondRenderCount === 1) { + expect(options).toEqual( + expect.objectContaining({ + requestKey: '1', + fetching: false, + data: { + books: [1, 42, 150] + }, + error: null, + failed: false, + url: '/test/succeeds/json-two' + }) + ); + } else if (secondRenderCount === 2) { + expect(options).toEqual( + expect.objectContaining({ + requestKey: '2', + fetching: true, + data: { + books: [1, 42, 150] + }, + error: null, + failed: false, + url: '/test/succeeds/json-two' + }) + ); + } else if (secondRenderCount === 3) { + expect(options).toEqual( + expect.objectContaining({ + requestKey: '2', + fetching: false, + data: { + authors: [22, 13] + }, + error: null, + failed: false, + url: '/test/succeeds/json-two' + }) + ); + } + } + }} + + ); + + setTimeout(() => { + // NOTE: this is for adding stuff to the cache. + // This DOES NOT test the cache-only behavior! + expect(fetchMock.calls('/test/succeeds/json-one').length).toBe(1); + expect(beforeFetchMock).toHaveBeenCalledTimes(1); + expect(afterFetchMock).toHaveBeenCalledTimes(1); + expect(afterFetchMock).toBeCalledWith( + expect.objectContaining({ + url: '/test/succeeds/json-one', + error: null, + failed: false, + didUnmount: false, + data: { + books: [1, 42, 150] + } + }) + ); + expect(onResponseMock).toHaveBeenCalledTimes(1); + expect(onResponseMock).toBeCalledWith( + null, + expect.objectContaining({ + ok: true, + status: 200, + statusText: 'OK', + data: { + books: [1, 42, 150] + } + }) + ); + + secondRun = true; + wrapper.setProps({ + url: '/test/succeeds/json-two', + requestKey: '2' + }); + + // We do a network timeout here to ensure that the `expect` within + // render is called a second time. + setTimeout(() => { + done(); + }, networkTimeout); + }, networkTimeout); + }); + + // test('it uses an indirectly-updated request key on subsequent renders', done => { + // // expect.assertions(10); + // const onResponseMock = jest.fn(); + // const beforeFetchMock = jest.fn(); + // const afterFetchMock = jest.fn(); + + // let run = 1; + + // const wrapper = mount( + // + // {options => { + // if (run === 1 && options.response) { + // expect(options.data).toEqual({ + // books: [1, 42, 150] + // }); + // } + // if (run === 2 && options.fetching) { + // expect(options.data).toEqual({ + // books: [1, 42, 150] + // }); + // } + + // if (run === 2 && !options.fetching) { + // expect(options.data).toEqual({ + // authors: [22, 13] + // }); + // } + + // // With data from the cache, there is no in-between render + // // for "loading." It just immediately gets set. + // if (run === 3) { + // expect(options.data).toEqual({ + // books: [1, 42, 150] + // }); + // } + // }} + // + // ); + + // setTimeout(() => { + // // NOTE: this is for adding stuff to the cache. + // // This DOES NOT test the cache-only behavior! + // expect(fetchMock.calls('/test/succeeds/json-one').length).toBe(1); + // expect(beforeFetchMock).toHaveBeenCalledTimes(1); + // expect(afterFetchMock).toHaveBeenCalledTimes(1); + // expect(afterFetchMock).toBeCalledWith( + // expect.objectContaining({ + // url: '/test/succeeds/json-one', + // error: null, + // failed: false, + // didUnmount: false, + // data: { + // books: [1, 42, 150] + // } + // }) + // ); + // expect(onResponseMock).toHaveBeenCalledTimes(1); + // expect(onResponseMock).toBeCalledWith( + // null, + // expect.objectContaining({ + // ok: true, + // status: 200, + // statusText: 'OK', + // data: { + // books: [1, 42, 150] + // } + // }) + // ); + + // run = 2; + // wrapper.setProps({ + // url: '/test/succeeds/json-two' + // }); + + // setTimeout(() => { + // run = 3; + // wrapper.setProps({ + // url: '/test/succeeds/json-one' + // }); + + // setTimeout(() => { + // done(); + // }, 500); + // }, 500); + // }, networkTimeout); + // }); + }); + + describe('doFetch', () => {}); +}); From 768163f9f14a71746666a964b437b1acf7536d13 Mon Sep 17 00:00:00 2001 From: James Smith Date: Wed, 18 Apr 2018 17:42:12 -0700 Subject: [PATCH 2/8] 2.0.4-beta.1 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index fbddcc2..18dc1d5 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "react-request", - "version": "2.0.3", + "version": "2.0.4-beta.1", "description": "Declarative HTTP requests with React.", "main": "lib/index.js", "module": "es/index.js", From f979a28974b1d5b701dcc986207d5474c21f3b4c Mon Sep 17 00:00:00 2001 From: James Smith Date: Thu, 19 Apr 2018 22:45:14 -0700 Subject: [PATCH 3/8] Store url in state --- src/fetch.js | 17 +- test/.eslintrc | 3 + test/do-fetch.test.js | 412 ++++++++++++++++++++++++++++++ test/index.test.js | 63 +---- test/responses.js | 7 + test/same-component.test.js | 496 ++++++++++++++++++++---------------- test/setup.js | 71 ++++++ 7 files changed, 785 insertions(+), 284 deletions(-) create mode 100644 test/do-fetch.test.js diff --git a/src/fetch.js b/src/fetch.js index 128e8aa..46407ba 100644 --- a/src/fetch.js +++ b/src/fetch.js @@ -13,8 +13,8 @@ export function clearResponseCache() { export class Fetch extends React.Component { render() { - const { children, requestName, url } = this.props; - const { fetching, response, data, error, requestKey } = this.state; + const { children, requestName } = this.props; + const { fetching, response, data, error, requestKey, url } = this.state; if (!children) { return null; @@ -49,7 +49,8 @@ export class Fetch extends React.Component { fetching: false, response: null, data: null, - error: null + error: null, + url: props.url || '' }; } @@ -197,10 +198,9 @@ export class Fetch extends React.Component { // `options` when calling `doFetch()`. Perhaps we should, however. const { requestName, dedupe, beforeFetch } = this.props; - this.cancelExistingRequest('New fetch initiated', this.props); - - const requestKey = this.getRequestKey(this.props, options); + this.cancelExistingRequest('New fetch initiated'); + const requestKey = this.getRequestKey(options); const requestOptions = Object.assign({}, this.props, options); const { @@ -249,6 +249,7 @@ export class Fetch extends React.Component { // If the request config changes, we need to be able to accurately // cancel the in-flight request. this.responseReceivedInfo = responseReceivedInfo; + this.hasHandledNetworkResponse = false; const fetchPolicy = this.getFetchPolicy(); @@ -287,6 +288,9 @@ export class Fetch extends React.Component { // may be duplicated in those situations, but that should be OK. It is necessary // to include this here to account for "network-only" fetch policies. requestKey, + url, + error: null, + failed: false, fetching: true }); const hittingNetwork = !isRequestInFlight(requestKey) || !dedupe; @@ -379,6 +383,7 @@ export class Fetch extends React.Component { this.setState( { + url, data, error, response, diff --git a/test/.eslintrc b/test/.eslintrc index 077076f..83e8b92 100644 --- a/test/.eslintrc +++ b/test/.eslintrc @@ -7,5 +7,8 @@ "ecmaFeatures": { "jsx": true } + }, + "globals": { + "hangingPromise": true } } diff --git a/test/do-fetch.test.js b/test/do-fetch.test.js new file mode 100644 index 0000000..2146656 --- /dev/null +++ b/test/do-fetch.test.js @@ -0,0 +1,412 @@ +import React from 'react'; +import fetchMock from 'fetch-mock'; +import { mount } from 'enzyme'; +import { Fetch, clearRequestCache, clearResponseCache } from '../src'; + +// Some time for the mock fetches to resolve +const networkTimeout = 10; + +beforeEach(() => { + clearRequestCache(); + clearResponseCache(); +}); + +describe('same-component doFetch() with caching (gh-151)', () => { + test('doFetch() with URL and another HTTP method', done => { + // expect.assertions(8); + const onResponseMock = jest.fn(); + const beforeFetchMock = jest.fn(); + const afterFetchMock = jest.fn(); + + let run = 1; + let renderCount = 0; + + mount( + + {options => { + renderCount++; + + // Wait for things to be placed in the cache. + // This occurs on the third render: + // 1st. component mounts + // 2nd. fetch begins + // 3rd. fetch ends + if (run === 1 && renderCount === 3) { + expect(options).toEqual(expect.objectContaining({ + fetching: false, + data: { + books: [1, 42, 150] + }, + error: null, + failed: false, + requestName: 'anonymousRequest', + url: '/test/succeeds/json-one' + })); + + // We need a timeout here to prevent a race condition + // with the assertions after the component mounts. + setTimeout(() => { + run++; + renderCount = 0; + options.doFetch({ + method: 'patch', + url: '/test/succeeds/patch' + }); + + // Now we need another timeout to allow for the fetch + // to occur. + setTimeout(() => { + done(); + }, networkTimeout); + }, networkTimeout * 2); + } + + if (run === 2) { + if (renderCount === 1) { + expect(options).toEqual(expect.objectContaining({ + fetching: true, + data: { + books: [1, 42, 150] + }, + error: null, + failed: false, + requestName: 'anonymousRequest', + url: '/test/succeeds/patch' + })); + } + if (renderCount === 2) { + expect(fetchMock.calls('/test/succeeds/patch').length).toBe(1); + expect(options).toEqual(expect.objectContaining({ + fetching: false, + data: { + movies: [1] + }, + error: null, + failed: false, + requestName: 'anonymousRequest', + url: '/test/succeeds/patch' + })); + } + if (renderCount === 3) { + done.fail(); + } + } + }} + + ); + + setTimeout(() => { + // NOTE: this is for adding stuff to the cache. + // This DOES NOT test the cache-only behavior! + expect(fetchMock.calls('/test/succeeds/json-one').length).toBe(1); + expect(beforeFetchMock).toHaveBeenCalledTimes(1); + expect(afterFetchMock).toHaveBeenCalledTimes(1); + expect(afterFetchMock).toBeCalledWith( + expect.objectContaining({ + url: '/test/succeeds/json-one', + error: null, + failed: false, + didUnmount: false, + data: { + books: [1, 42, 150] + } + }) + ); + expect(onResponseMock).toHaveBeenCalledTimes(1); + expect(onResponseMock).toBeCalledWith( + null, + expect.objectContaining({ + ok: true, + status: 200, + statusText: 'OK', + data: { + books: [1, 42, 150] + } + }) + ); + }, networkTimeout); + }); + + test('doFetch() with request key and URL', done => { + // expect.assertions(8); + const onResponseMock = jest.fn(); + const beforeFetchMock = jest.fn(); + const afterFetchMock = jest.fn(); + + let run = 1; + let renderCount = 0; + + mount( + + {options => { + renderCount++; + + // Wait for things to be placed in the cache. + // This occurs on the third render: + // 1st. component mounts + // 2nd. fetch begins + // 3rd. fetch ends + if (run === 1 && renderCount === 3) { + expect(options).toEqual(expect.objectContaining({ + fetching: false, + data: { + books: [1, 42, 150] + }, + error: null, + failed: false, + requestName: 'anonymousRequest', + url: '/test/succeeds/json-one' + })); + + // We need a timeout here to prevent a race condition + // with the assertions after the component mounts. + setTimeout(() => { + run++; + renderCount = 0; + + options.doFetch({ + requestKey: 'sandwiches', + url: '/test/succeeds/json-two' + }); + + // Now we need another timeout to allow for the fetch + // to occur. + setTimeout(() => { + done(); + }, networkTimeout); + }, networkTimeout * 2); + } + + if (run === 2) { + if (renderCount === 1) { + expect(options).toEqual(expect.objectContaining({ + fetching: true, + data: { + books: [1, 42, 150] + }, + error: null, + failed: false, + requestKey: 'sandwiches', + requestName: 'anonymousRequest', + url: '/test/succeeds/json-two' + })); + } + if (renderCount === 2) { + expect(options).toEqual(expect.objectContaining({ + fetching: false, + data: { + authors: [22, 13] + }, + error: null, + failed: false, + requestKey: 'sandwiches', + requestName: 'anonymousRequest', + url: '/test/succeeds/json-two' + })); + } + if (renderCount === 3) { + done.fail(); + } + } + }} + + ); + + setTimeout(() => { + // NOTE: this is for adding stuff to the cache. + // This DOES NOT test the cache-only behavior! + expect(fetchMock.calls('/test/succeeds/json-one').length).toBe(1); + expect(beforeFetchMock).toHaveBeenCalledTimes(1); + expect(afterFetchMock).toHaveBeenCalledTimes(1); + expect(afterFetchMock).toBeCalledWith( + expect.objectContaining({ + url: '/test/succeeds/json-one', + error: null, + failed: false, + didUnmount: false, + data: { + books: [1, 42, 150] + } + }) + ); + expect(onResponseMock).toHaveBeenCalledTimes(1); + expect(onResponseMock).toBeCalledWith( + null, + expect.objectContaining({ + ok: true, + status: 200, + statusText: 'OK', + data: { + books: [1, 42, 150] + } + }) + ); + }, networkTimeout); + }); + + // Note: this does not test dedupe due to the fact that the requests + // resolve too quickly. + test('doFetch(); testing cancelation', done => { + // expect.assertions(8); + const onResponseMock = jest.fn(); + const beforeFetchMock = jest.fn(); + const afterFetchMock = jest.fn(); + + let run = 1; + let renderCount = 0; + + mount( + + {options => { + renderCount++; + + // Wait for things to be placed in the cache. + // This occurs on the third render: + // 1st. component mounts + // 2nd. fetch begins + // 3rd. fetch ends + if (run === 1 && renderCount === 3) { + expect(options).toEqual(expect.objectContaining({ + fetching: false, + data: { + books: [1, 42, 150] + }, + error: null, + failed: false, + requestName: 'anonymousRequest', + url: '/test/succeeds/json-one' + })); + + // We need a timeout here to prevent a race condition + // with the assertions after the component mounts. + setTimeout(() => { + run++; + renderCount = 0; + + options.doFetch({ + requestKey: 'sandwiches', + url: '/test/succeeds/json-two' + }); + + options.doFetch({ + requestKey: 'sandwiches', + url: '/test/succeeds/json-two' + }); + + // Now we need another timeout to allow for the fetch + // to occur. + setTimeout(() => { + done(); + }, networkTimeout); + }, networkTimeout * 2); + } + + if (run === 2) { + if (renderCount === 1) { + expect(options).toEqual(expect.objectContaining({ + fetching: true, + data: { + books: [1, 42, 150] + }, + error: null, + failed: false, + requestKey: 'sandwiches', + requestName: 'anonymousRequest', + url: '/test/succeeds/json-two' + })); + } + if (renderCount === 2) { + expect(options).toEqual(expect.objectContaining({ + fetching: false, + // I am not sure if I like this behavior! + // See gh-154 for more + data: null, + failed: true, + requestKey: 'sandwiches', + requestName: 'anonymousRequest', + url: '/test/succeeds/json-two' + })); + } + + // This is the 2nd doFetch(). It is difficult to update + // the `run` for that fetch, so we just use the renderCounts. + else if (renderCount === 3) { + expect(options).toEqual(expect.objectContaining({ + fetching: true, + data: null, + error: null, + failed: false, + requestKey: 'sandwiches', + requestName: 'anonymousRequest', + url: '/test/succeeds/json-two' + })); + } + + else if (renderCount === 4) { + expect(options).toEqual(expect.objectContaining({ + fetching: false, + data: { + authors: [22, 13] + }, + error: null, + failed: false, + requestKey: 'sandwiches', + requestName: 'anonymousRequest', + url: '/test/succeeds/json-two' + })); + } + if (renderCount > 4) { + done.fail(); + } + } + }} + + ); + + setTimeout(() => { + // NOTE: this is for adding stuff to the cache. + // This DOES NOT test the cache-only behavior! + expect(fetchMock.calls('/test/succeeds/json-one').length).toBe(1); + expect(beforeFetchMock).toHaveBeenCalledTimes(1); + expect(afterFetchMock).toHaveBeenCalledTimes(1); + expect(afterFetchMock).toBeCalledWith( + expect.objectContaining({ + url: '/test/succeeds/json-one', + error: null, + failed: false, + didUnmount: false, + data: { + books: [1, 42, 150] + } + }) + ); + expect(onResponseMock).toHaveBeenCalledTimes(1); + expect(onResponseMock).toBeCalledWith( + null, + expect.objectContaining({ + ok: true, + status: 200, + statusText: 'OK', + data: { + books: [1, 42, 150] + } + }) + ); + }, networkTimeout); + }); +}); \ No newline at end of file diff --git a/test/index.test.js b/test/index.test.js index 6bb8520..a617e86 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -7,28 +7,7 @@ import { getRequestKey, clearResponseCache } from '../src'; -import { successfulResponse, jsonResponse, jsonResponse2 } from './responses'; -import { setTimeout } from 'timers'; - -function hangingPromise() { - return new Promise(() => {}); -} - -fetchMock.get('/test/hangs', hangingPromise()); -fetchMock.get('/test/hangs/1', hangingPromise()); -fetchMock.get('/test/hangs/2', hangingPromise()); -fetchMock.post('/test/hangs', hangingPromise()); -fetchMock.put('/test/hangs', hangingPromise()); -fetchMock.patch('/test/hangs', hangingPromise()); -fetchMock.head('/test/hangs', hangingPromise()); -fetchMock.delete('/test/hangs', hangingPromise()); - -// This could be improved by adding the URL to the JSON response -fetchMock.get('/test/succeeds', () => { - return new Promise(resolve => { - resolve(jsonResponse()); - }); -}); +import { successfulResponse, jsonResponse } from './responses'; let success = true; // This could be improved by adding the URL to the JSON response @@ -46,46 +25,6 @@ fetchMock.get('/test/variable', () => { } }); -fetchMock.get( - '/test/succeeds/cache-only-empty', - () => - new Promise(resolve => { - resolve(successfulResponse()); - }) -); - -fetchMock.get( - '/test/succeeds/cache-only-full', - () => - new Promise(resolve => { - resolve(jsonResponse()); - }) -); - -fetchMock.post( - '/test/succeeds/cache-only-full', - () => - new Promise(resolve => { - resolve(jsonResponse()); - }) -); - -fetchMock.get( - '/test/succeeds/json-one', - () => - new Promise(resolve => { - resolve(jsonResponse()); - }) -); - -fetchMock.get( - '/test/succeeds/json-two', - () => - new Promise(resolve => { - resolve(jsonResponse2()); - }) -); - // Some time for the mock fetches to resolve const networkTimeout = 10; diff --git a/test/responses.js b/test/responses.js index 723b08e..ffe0ae1 100644 --- a/test/responses.js +++ b/test/responses.js @@ -18,3 +18,10 @@ export function jsonResponse2() { statusText: 'OK' }); } + +export function jsonResponse3() { + return new Response('{"movies": [1]}', { + status: 200, + statusText: 'OK' + }); +} \ No newline at end of file diff --git a/test/same-component.test.js b/test/same-component.test.js index 27db924..0d6f2b7 100644 --- a/test/same-component.test.js +++ b/test/same-component.test.js @@ -2,24 +2,6 @@ import React from 'react'; import fetchMock from 'fetch-mock'; import { mount } from 'enzyme'; import { Fetch, clearRequestCache, clearResponseCache } from '../src'; -import { jsonResponse, jsonResponse2 } from './responses'; -import { setTimeout } from 'timers'; - -fetchMock.get( - '/test/succeeds/json-one', - () => - new Promise(resolve => { - resolve(jsonResponse()); - }) -); - -fetchMock.get( - '/test/succeeds/json-two', - () => - new Promise(resolve => { - resolve(jsonResponse2()); - }) -); // Some time for the mock fetches to resolve const networkTimeout = 10; @@ -29,216 +11,298 @@ beforeEach(() => { clearResponseCache(); }); +// Issue 151 describes the 3 situations when requests can be made. This tests +// situation 2. describe('same-component subsequent requests with caching (gh-151)', () => { - // Issue 151 describes the 3 situations when requests can be made. "Prop changes" - // refers to situation 2. - describe('prop changes', () => { - test('it uses a directly-updated request key on subsequent renders', done => { - // expect.assertions(8); - const onResponseMock = jest.fn(); - const beforeFetchMock = jest.fn(); - const afterFetchMock = jest.fn(); - - let secondRun = false; - let secondRenderCount = 0; - - const wrapper = mount( - - {options => { - if (secondRun) { - // Increment our render count. This allows us to - // test for each of the individual renders involved - // with changing the prop. - secondRenderCount++; - - // This first render is interesting: we basically only have a - // new URL set, but the request has not yet begun. The reason - // for this is because we do the fetch in `componentDidUpdate`. - if (secondRenderCount === 1) { - expect(options).toEqual( - expect.objectContaining({ - requestKey: '1', - fetching: false, - data: { - books: [1, 42, 150] - }, - error: null, - failed: false, - url: '/test/succeeds/json-two' - }) - ); - } else if (secondRenderCount === 2) { - expect(options).toEqual( - expect.objectContaining({ - requestKey: '2', - fetching: true, - data: { - books: [1, 42, 150] - }, - error: null, - failed: false, - url: '/test/succeeds/json-two' - }) - ); - } else if (secondRenderCount === 3) { - expect(options).toEqual( - expect.objectContaining({ - requestKey: '2', - fetching: false, - data: { - authors: [22, 13] - }, - error: null, - failed: false, - url: '/test/succeeds/json-two' - }) - ); - } + test('it uses a directly-updated request key on subsequent renders', done => { + // expect.assertions(8); + const onResponseMock = jest.fn(); + const beforeFetchMock = jest.fn(); + const afterFetchMock = jest.fn(); + + let run = 1; + let renderCount = 0; + + const wrapper = mount( + + {options => { + if (run === 2) { + // Increment our render count. This allows us to + // test for each of the individual renders involved + // with changing the prop. + renderCount++; + + // This first render is interesting: we basically only have a + // new URL set, but the request has not yet begun. The reason + // for this is because we do the fetch in `componentDidUpdate`. + if (renderCount === 1) { + expect(options).toEqual( + expect.objectContaining({ + requestKey: '1', + fetching: false, + data: { + books: [1, 42, 150] + }, + error: null, + failed: false, + url: '/test/succeeds/json-one' + }) + ); + } else if (renderCount === 2) { + expect(options).toEqual( + expect.objectContaining({ + requestKey: '2', + fetching: true, + data: { + books: [1, 42, 150] + }, + error: null, + failed: false, + url: '/test/succeeds/json-two' + }) + ); + } else if (renderCount === 3) { + expect(options).toEqual( + expect.objectContaining({ + requestKey: '2', + fetching: false, + data: { + authors: [22, 13] + }, + error: null, + failed: false, + url: '/test/succeeds/json-two' + }) + ); + } else if (renderCount > 3) { + done.fail(); } - }} - + } + }} + + ); + + setTimeout(() => { + // NOTE: this is for adding stuff to the cache. + // This DOES NOT test the cache-only behavior! + expect(fetchMock.calls('/test/succeeds/json-one').length).toBe(1); + expect(beforeFetchMock).toHaveBeenCalledTimes(1); + expect(afterFetchMock).toHaveBeenCalledTimes(1); + expect(afterFetchMock).toBeCalledWith( + expect.objectContaining({ + url: '/test/succeeds/json-one', + error: null, + failed: false, + didUnmount: false, + data: { + books: [1, 42, 150] + } + }) + ); + expect(onResponseMock).toHaveBeenCalledTimes(1); + expect(onResponseMock).toBeCalledWith( + null, + expect.objectContaining({ + ok: true, + status: 200, + statusText: 'OK', + data: { + books: [1, 42, 150] + } + }) ); + run = 2; + wrapper.setProps({ + url: '/test/succeeds/json-two', + requestKey: '2' + }); + + // We do a network timeout here to ensure that the `expect` within + // render is called a second time. setTimeout(() => { - // NOTE: this is for adding stuff to the cache. - // This DOES NOT test the cache-only behavior! - expect(fetchMock.calls('/test/succeeds/json-one').length).toBe(1); - expect(beforeFetchMock).toHaveBeenCalledTimes(1); - expect(afterFetchMock).toHaveBeenCalledTimes(1); - expect(afterFetchMock).toBeCalledWith( - expect.objectContaining({ - url: '/test/succeeds/json-one', - error: null, - failed: false, - didUnmount: false, - data: { - books: [1, 42, 150] + done(); + }, networkTimeout); + }, networkTimeout); + }); + + test('it uses an indirectly-updated request key on subsequent renders', done => { + // expect.assertions(10); + const onResponseMock = jest.fn(); + const beforeFetchMock = jest.fn(); + const afterFetchMock = jest.fn(); + + let run = 1; + let renderCount = 0; + + const wrapper = mount( + + {(options) => { + renderCount++; + if (run === 1) { + if (renderCount === 1) { + expect(options).toEqual(expect.objectContaining({ + fetching: false, + data: null, + error: null, + failed: false, + response: null, + requestName: 'anonymousRequest', + url: '/test/succeeds/json-one' + })); + } else if (renderCount === 2) { + expect(options).toEqual(expect.objectContaining({ + fetching: true, + data: null, + error: null, + failed: false, + response: null, + requestName: 'anonymousRequest', + url: '/test/succeeds/json-one' + })); + } else if (renderCount === 3) { + expect(options).toEqual(expect.objectContaining({ + fetching: false, + data: { + books: [1, 42, 150] + }, + error: null, + failed: false, + requestName: 'anonymousRequest', + url: '/test/succeeds/json-one' + })); + } else if (renderCount > 3) { + done.fail(); } - }) - ); - expect(onResponseMock).toHaveBeenCalledTimes(1); - expect(onResponseMock).toBeCalledWith( - null, - expect.objectContaining({ - ok: true, - status: 200, - statusText: 'OK', - data: { - books: [1, 42, 150] + } + + else if (run === 2) { + if (renderCount === 1) { + expect(options).toEqual(expect.objectContaining({ + fetching: false, + data: { + books: [1, 42, 150] + }, + error: null, + failed: false, + requestName: 'anonymousRequest', + url: '/test/succeeds/json-one' + })); + } else if (renderCount === 2) { + expect(options).toEqual(expect.objectContaining({ + fetching: true, + data: { + books: [1, 42, 150] + }, + error: null, + failed: false, + requestName: 'anonymousRequest', + url: '/test/succeeds/json-two' + })); + } else if (renderCount === 3) { + expect(options).toEqual(expect.objectContaining({ + fetching: false, + data: { + authors: [22, 13] + }, + error: null, + failed: false, + requestName: 'anonymousRequest', + url: '/test/succeeds/json-two' + })); + } else if (renderCount > 3) { + done.fail(); } - }) - ); + } - secondRun = true; + else if (run === 3) { + if (renderCount === 1) { + expect(options).toEqual(expect.objectContaining({ + fetching: false, + data: { + authors: [22, 13] + }, + error: null, + failed: false, + requestName: 'anonymousRequest', + url: '/test/succeeds/json-two' + })); + } + else if (renderCount === 2) { + expect(options).toEqual(expect.objectContaining({ + fetching: false, + data: { + books: [1, 42, 150] + }, + error: null, + failed: false, + requestName: 'anonymousRequest', + url: '/test/succeeds/json-one' + })); + } else if (renderCount > 2) { + done.fail(); + } + } + }} + + ); + + setTimeout(() => { + // NOTE: this is for adding stuff to the cache. + // This DOES NOT test the cache-only behavior! + expect(fetchMock.calls('/test/succeeds/json-one').length).toBe(1); + expect(beforeFetchMock).toHaveBeenCalledTimes(1); + expect(afterFetchMock).toHaveBeenCalledTimes(1); + expect(afterFetchMock).toBeCalledWith( + expect.objectContaining({ + url: '/test/succeeds/json-one', + error: null, + failed: false, + didUnmount: false, + data: { + books: [1, 42, 150] + } + }) + ); + expect(onResponseMock).toHaveBeenCalledTimes(1); + expect(onResponseMock).toBeCalledWith( + null, + expect.objectContaining({ + ok: true, + status: 200, + statusText: 'OK', + data: { + books: [1, 42, 150] + } + }) + ); + + run = 2; + renderCount = 0; + wrapper.setProps({ + url: '/test/succeeds/json-two' + }); + + setTimeout(() => { + run = 3; + renderCount = 0; wrapper.setProps({ - url: '/test/succeeds/json-two', - requestKey: '2' + url: '/test/succeeds/json-one' }); - // We do a network timeout here to ensure that the `expect` within - // render is called a second time. setTimeout(() => { done(); - }, networkTimeout); - }, networkTimeout); - }); - - // test('it uses an indirectly-updated request key on subsequent renders', done => { - // // expect.assertions(10); - // const onResponseMock = jest.fn(); - // const beforeFetchMock = jest.fn(); - // const afterFetchMock = jest.fn(); - - // let run = 1; - - // const wrapper = mount( - // - // {options => { - // if (run === 1 && options.response) { - // expect(options.data).toEqual({ - // books: [1, 42, 150] - // }); - // } - // if (run === 2 && options.fetching) { - // expect(options.data).toEqual({ - // books: [1, 42, 150] - // }); - // } - - // if (run === 2 && !options.fetching) { - // expect(options.data).toEqual({ - // authors: [22, 13] - // }); - // } - - // // With data from the cache, there is no in-between render - // // for "loading." It just immediately gets set. - // if (run === 3) { - // expect(options.data).toEqual({ - // books: [1, 42, 150] - // }); - // } - // }} - // - // ); - - // setTimeout(() => { - // // NOTE: this is for adding stuff to the cache. - // // This DOES NOT test the cache-only behavior! - // expect(fetchMock.calls('/test/succeeds/json-one').length).toBe(1); - // expect(beforeFetchMock).toHaveBeenCalledTimes(1); - // expect(afterFetchMock).toHaveBeenCalledTimes(1); - // expect(afterFetchMock).toBeCalledWith( - // expect.objectContaining({ - // url: '/test/succeeds/json-one', - // error: null, - // failed: false, - // didUnmount: false, - // data: { - // books: [1, 42, 150] - // } - // }) - // ); - // expect(onResponseMock).toHaveBeenCalledTimes(1); - // expect(onResponseMock).toBeCalledWith( - // null, - // expect.objectContaining({ - // ok: true, - // status: 200, - // statusText: 'OK', - // data: { - // books: [1, 42, 150] - // } - // }) - // ); - - // run = 2; - // wrapper.setProps({ - // url: '/test/succeeds/json-two' - // }); - - // setTimeout(() => { - // run = 3; - // wrapper.setProps({ - // url: '/test/succeeds/json-one' - // }); - - // setTimeout(() => { - // done(); - // }, 500); - // }, 500); - // }, networkTimeout); - // }); + }, 500); + }, 500); + }, networkTimeout); }); - - describe('doFetch', () => {}); }); diff --git a/test/setup.js b/test/setup.js index ef58578..46e3654 100644 --- a/test/setup.js +++ b/test/setup.js @@ -2,6 +2,9 @@ import 'isomorphic-fetch'; import fetchMock from 'fetch-mock'; import Enzyme from 'enzyme'; import Adapter from 'enzyme-adapter-react-16'; +import { + successfulResponse, jsonResponse, jsonResponse2, jsonResponse3 +} from './responses'; Enzyme.configure({ adapter: new Adapter() }); @@ -9,6 +12,74 @@ Enzyme.configure({ adapter: new Adapter() }); // an error. global.AbortSignal = function() {}; +var hangingPromise = global.hangingPromise = function() { + return new Promise(() => {}); +} + +fetchMock.get('/test/hangs', hangingPromise()); +fetchMock.get('/test/hangs/1', hangingPromise()); +fetchMock.get('/test/hangs/2', hangingPromise()); +fetchMock.post('/test/hangs', hangingPromise()); +fetchMock.put('/test/hangs', hangingPromise()); +fetchMock.patch('/test/hangs', hangingPromise()); +fetchMock.head('/test/hangs', hangingPromise()); +fetchMock.delete('/test/hangs', hangingPromise()); + +// This could be improved by adding the URL to the JSON response +fetchMock.get('/test/succeeds', () => { + return new Promise(resolve => { + resolve(jsonResponse()); + }); +}); + +fetchMock.get( + '/test/succeeds/cache-only-empty', + () => + new Promise(resolve => { + resolve(successfulResponse()); + }) +); + +fetchMock.get( + '/test/succeeds/cache-only-full', + () => + new Promise(resolve => { + resolve(jsonResponse()); + }) +); + +fetchMock.post( + '/test/succeeds/cache-only-full', + () => + new Promise(resolve => { + resolve(jsonResponse()); + }) +); + +fetchMock.get( + '/test/succeeds/json-one', + () => + new Promise(resolve => { + resolve(jsonResponse()); + }) +); + +fetchMock.get( + '/test/succeeds/json-two', + () => + new Promise(resolve => { + resolve(jsonResponse2()); + }) +); + +fetchMock.patch( + '/test/succeeds/patch', + () => + new Promise(resolve => { + resolve(jsonResponse3()); + }) +); + // We do this at the start of each test, just in case a test // replaces the global fetch and does not reset it beforeEach(() => { From 0d57b854d2073db349d18a8d688bbfc0ea0d7169 Mon Sep 17 00:00:00 2001 From: James Smith Date: Thu, 19 Apr 2018 23:46:09 -0700 Subject: [PATCH 4/8] 2.0.4-beta.2 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 18dc1d5..239a5db 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "react-request", - "version": "2.0.4-beta.1", + "version": "2.0.4-beta.2", "description": "Declarative HTTP requests with React.", "main": "lib/index.js", "module": "es/index.js", From 39de3f8b900e85f3ec437a177071bbd724d7a082 Mon Sep 17 00:00:00 2001 From: James Smith Date: Thu, 19 Apr 2018 23:59:42 -0700 Subject: [PATCH 5/8] No longer set a default URL string --- src/fetch.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fetch.js b/src/fetch.js index 46407ba..3110e6a 100644 --- a/src/fetch.js +++ b/src/fetch.js @@ -50,7 +50,7 @@ export class Fetch extends React.Component { response: null, data: null, error: null, - url: props.url || '' + url: props.url }; } From 7460f82d67e6512e82cebfbaf072c351d4a8a2f1 Mon Sep 17 00:00:00 2001 From: James Smith Date: Fri, 20 Apr 2018 00:00:17 -0700 Subject: [PATCH 6/8] 2.0.4-beta.3 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 239a5db..06b7df3 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "react-request", - "version": "2.0.4-beta.2", + "version": "2.0.4-beta.3", "description": "Declarative HTTP requests with React.", "main": "lib/index.js", "module": "es/index.js", From 371f44f0301098d564885cc997e33a4101a038c3 Mon Sep 17 00:00:00 2001 From: James Smith Date: Fri, 20 Apr 2018 09:31:03 -0700 Subject: [PATCH 7/8] Update code comments --- src/fetch.js | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/fetch.js b/src/fetch.js index 3110e6a..1e53c6c 100644 --- a/src/fetch.js +++ b/src/fetch.js @@ -7,12 +7,20 @@ import { getRequestKey, fetchDedupe, isRequestInFlight } from 'fetch-dedupe'; // The value of each key is a Response instance let responseCache = {}; +// The docs state that this is not safe to use in an +// application. That's just because I am not writing tests, +// nor designing the API, around folks clearing the cache. +// This was only added to help out with testing your app. +// Use your judgment if you decide to use this in your +// app directly. export function clearResponseCache() { responseCache = {}; } export class Fetch extends React.Component { render() { + // Anything pulled from `this.props` here is not eligible to be + // specified when calling `doFetch`. const { children, requestName } = this.props; const { fetching, response, data, error, requestKey, url } = this.state; @@ -96,6 +104,9 @@ export class Fetch extends React.Component { } } + // Because we use `componentDidUpdate` to determine if we should fetch + // again, there will be at least one render when you receive your new + // fetch options, such as a new URL, but the fetch has not begun yet. componentDidUpdate(prevProps) { const currentRequestKey = this.props.requestKey || @@ -103,7 +114,6 @@ export class Fetch extends React.Component { ...this.props, method: this.props.method.toUpperCase() }); - // const prevRequestKey = prevProps.requestKey || getRequestKey({ @@ -128,6 +138,8 @@ export class Fetch extends React.Component { cancelExistingRequest = reason => { if (this.state.fetching && !this.hasHandledNetworkResponse) { const abortError = new Error(reason); + // This is an effort to mimic the error that is created when a + // fetch is actually aborted using the AbortController API. abortError.name = 'AbortError'; this.onResponseReceived({ ...this.responseReceivedInfo, @@ -190,9 +202,6 @@ export class Fetch extends React.Component { } }; - // Heads up: accessing `this.props` within `fetchData` - // is unsafe! This is because we call `fetchData` from - // within `componentWillReceiveProps`! fetchData = (options, ignoreCache) => { // These are the things that we do not allow a user to configure in // `options` when calling `doFetch()`. Perhaps we should, however. @@ -283,10 +292,6 @@ export class Fetch extends React.Component { } this.setState({ - // Note: when data is returned from the cache, the calls to `onResponseReceived` - // will call `setState` to update the `requestKey`. This call to update `requestKey` - // may be duplicated in those situations, but that should be OK. It is necessary - // to include this here to account for "network-only" fetch policies. requestKey, url, error: null, From f9759f9e3fae241af4464203bea833a67d0a01a0 Mon Sep 17 00:00:00 2001 From: James Smith Date: Fri, 20 Apr 2018 10:03:09 -0700 Subject: [PATCH 8/8] 2.0.4 --- CHANGELOG.md | 7 +++++++ package.json | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 935eeea..cc4579b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # Changelog +### v2.0.4 (2018/4/20) + +**Bug Fixes** + +* Fixes a bug where there could be a cache mismatch when re-rendering the same component + that has a fetch policy configured. + ### v2.0.3 (2018/3/2) **Bug Fixes** diff --git a/package.json b/package.json index 06b7df3..bf99492 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "react-request", - "version": "2.0.4-beta.3", + "version": "2.0.4", "description": "Declarative HTTP requests with React.", "main": "lib/index.js", "module": "es/index.js",