Skip to content

Commit

Permalink
Fix useRequest to support query change (#57723) (#58067)
Browse files Browse the repository at this point in the history
  • Loading branch information
nchaulet authored Feb 20, 2020
1 parent 1f87879 commit 30cc81f
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 12 deletions.
25 changes: 18 additions & 7 deletions src/plugins/es_ui_shared/public/request/np_ready_request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import { useEffect, useState, useRef } from 'react';
import { useEffect, useState, useRef, useMemo } from 'react';

import { HttpSetup, HttpFetchQuery } from '../../../../../src/core/public';

Expand Down Expand Up @@ -78,6 +78,8 @@ export const useRequest = (
deserializer = (data: any): any => data,
}: UseRequestConfig
): UseRequestResponse => {
const sendRequestRef = useRef<() => Promise<SendRequestResponse>>();

// Main states for tracking request status and data
const [error, setError] = useState<null | any>(null);
const [isLoading, setIsLoading] = useState<boolean>(true);
Expand All @@ -102,7 +104,10 @@ export const useRequest = (

// Set new interval
if (pollInterval.current) {
pollIntervalId.current = setTimeout(_sendRequest, pollInterval.current);
pollIntervalId.current = setTimeout(
() => (sendRequestRef.current ?? _sendRequest)(),
pollInterval.current
);
}
};

Expand Down Expand Up @@ -145,11 +150,17 @@ export const useRequest = (
};

useEffect(() => {
_sendRequest();
// To be functionally correct we'd send a new request if the method, path, or body changes.
sendRequestRef.current = _sendRequest;
}, [_sendRequest]);

const stringifiedQuery = useMemo(() => JSON.stringify(query), [query]);

useEffect(() => {
(sendRequestRef.current ?? _sendRequest)();
// To be functionally correct we'd send a new request if the method, path, query or body changes.
// But it doesn't seem likely that the method will change and body is likely to be a new
// object even if its shape hasn't changed, so for now we're just watching the path.
}, [path]);
// object even if its shape hasn't changed, so for now we're just watching the path and the query.
}, [path, stringifiedQuery]);

useEffect(() => {
scheduleRequest();
Expand All @@ -168,6 +179,6 @@ export const useRequest = (
isLoading,
error,
data,
sendRequest: _sendRequest, // Gives the user the ability to manually request data
sendRequest: sendRequestRef.current ?? _sendRequest, // Gives the user the ability to manually request data
};
};
10 changes: 5 additions & 5 deletions src/plugins/es_ui_shared/public/request/request.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ describe.skip('request lib', () => {
it('uses the provided path, method, and body to send the request', async () => {
const response = await sendRequest({ ...successRequest });
sinon.assert.calledOnce(sendPost);
expect(response).toEqual({ data: successResponse.data });
expect(response).toEqual({ data: successResponse.data, error: null });
});

it('surfaces errors', async () => {
Expand Down Expand Up @@ -182,11 +182,11 @@ describe.skip('request lib', () => {
expect(hook.error).toBe(errorResponse);
});

it('is undefined when the request is successful', async () => {
it('is null when the request is successful', async () => {
initUseRequest({ ...successRequest });
await wait(50);
expect(hook.isLoading).toBe(false);
expect(hook.error).toBeUndefined();
expect(hook.error).toBeNull();
});
});

Expand All @@ -205,11 +205,11 @@ describe.skip('request lib', () => {
expect(hook.data).toBe(successResponse.data);
});

it('is undefined when the request fails', async () => {
it('is null when the request fails', async () => {
initUseRequest({ ...errorRequest });
await wait(50);
expect(hook.isLoading).toBe(false);
expect(hook.data).toBeUndefined();
expect(hook.data).toBeNull();
});
});
});
Expand Down

0 comments on commit 30cc81f

Please sign in to comment.