Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.x] Fix useRequest to support query change (#57723) #58067

Merged
merged 2 commits into from
Feb 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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