From 9b142107c3813f4b8bc12c4e2280992e87f91d7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien=20Loix?= Date: Tue, 4 Aug 2020 15:46:59 +0200 Subject: [PATCH 1/5] Use handler to provide hook result update --- .../public/request/use_request.helpers.tsx | 68 +++++++++++-------- 1 file changed, 39 insertions(+), 29 deletions(-) diff --git a/src/plugins/es_ui_shared/public/request/use_request.helpers.tsx b/src/plugins/es_ui_shared/public/request/use_request.helpers.tsx index 3402ae8dab55cb..0ca3fb9a401fc5 100644 --- a/src/plugins/es_ui_shared/public/request/use_request.helpers.tsx +++ b/src/plugins/es_ui_shared/public/request/use_request.helpers.tsx @@ -17,7 +17,7 @@ * under the License. */ -import React from 'react'; +import React, { useEffect } from 'react'; import { act } from 'react-dom/test-utils'; import { mount, ReactWrapper } from 'enzyme'; import sinon from 'sinon'; @@ -86,40 +86,50 @@ export const createUseRequestHelpers = (): UseRequestHelpers => { const hookResult = {} as UseRequestResponse; const sendRequestSpy = sinon.stub(); - const setupUseRequest = (config: SendRequestConfig) => { - const httpClient = { - post: (path: string, options: HttpFetchOptions) => { - return new Promise((resolve, reject) => { - // Increase the time it takes to resolve a request so we have time to inspect the hook - // as it goes through various states. - setTimeout(() => { - try { - resolve(sendRequestSpy(path, options)); - } catch (e) { - reject(e); - } - }, REQUEST_TIME); - }); - }, - }; - - const TestComponent = ({ requestConfig }: { requestConfig: SendRequestConfig }) => { - const { isInitialRequest, isLoading, error, data, sendRequest } = useRequest( - httpClient as HttpSetup, - requestConfig - ); + const httpClient = { + post: (path: string, options: HttpFetchOptions) => { + return new Promise((resolve, reject) => { + // Increase the time it takes to resolve a request so we have time to inspect the hook + // as it goes through various states. + setTimeout(() => { + try { + resolve(sendRequestSpy(path, options)); + } catch (e) { + reject(e); + } + }, REQUEST_TIME); + }); + }, + }; - hookResult.isInitialRequest = isInitialRequest; - hookResult.isLoading = isLoading; - hookResult.error = error; - hookResult.data = data; - hookResult.sendRequest = sendRequest; + const setupUseRequest = ( + config: SendRequestConfig, + onChange: (hookData: UseRequestResponse) => void = (hookData) => { + hookResult.isInitialRequest = hookData.isInitialRequest; + hookResult.isLoading = hookData.isLoading; + hookResult.error = hookData.error; + hookResult.data = hookData.data; + hookResult.sendRequest = hookData.sendRequest; + } + ) => { + const TestComponent = ({ + requestConfig, + onHookChange, + }: { + requestConfig: SendRequestConfig; + onHookChange: (hookData: UseRequestResponse) => void; + }) => { + const hookData = useRequest(httpClient as HttpSetup, requestConfig); + + useEffect(() => { + onHookChange(hookData); + }, [hookData, onHookChange]); return null; }; act(() => { - element = mount(); + element = mount(); }); }; From 3391fcfaacd389f8fb53cc3a8ed63465d8c36c25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien=20Loix?= Date: Tue, 4 Aug 2020 19:48:17 +0200 Subject: [PATCH 2/5] Refactor useRequest() hook to use callBacks instead of refs --- .../public/request/use_request.test.ts | 4 +- .../public/request/use_request.ts | 124 ++++++++++-------- 2 files changed, 72 insertions(+), 56 deletions(-) diff --git a/src/plugins/es_ui_shared/public/request/use_request.test.ts b/src/plugins/es_ui_shared/public/request/use_request.test.ts index 41cebdb3598a29..8d25136b620d22 100644 --- a/src/plugins/es_ui_shared/public/request/use_request.test.ts +++ b/src/plugins/es_ui_shared/public/request/use_request.test.ts @@ -236,7 +236,7 @@ describe('useRequest hook', () => { // because we want to assert against the hook state while the Promise is pending. const { setupSuccessRequest, advanceTime, hookResult, getSendRequestSpy } = helpers; - const DOUBLE_REQUEST_TIME = REQUEST_TIME * 2; + const DOUBLE_REQUEST_TIME = REQUEST_TIME * 2 + 50; // We add 50 to avoid a race condition setupSuccessRequest({ pollIntervalMs: DOUBLE_REQUEST_TIME }); // The initial request resolves, and then we'll immediately send a new one manually... @@ -253,7 +253,7 @@ describe('useRequest hook', () => { hookResult.sendRequest(); }); - // At this point, we've moved forward 3s. The poll is set at 2s. If sendRequest didn't + // At this point, we've moved forward 3000 ms. The poll is set at 2050s. If sendRequest didn't // reset the poll, the request call count would be 4, not 3. await advanceTime(REQUEST_TIME); expect(getSendRequestSpy().callCount).toBe(3); diff --git a/src/plugins/es_ui_shared/public/request/use_request.ts b/src/plugins/es_ui_shared/public/request/use_request.ts index f7467b69cf6416..e7f99d583379af 100644 --- a/src/plugins/es_ui_shared/public/request/use_request.ts +++ b/src/plugins/es_ui_shared/public/request/use_request.ts @@ -17,10 +17,14 @@ * under the License. */ -import { useEffect, useState, useRef } from 'react'; +import { useEffect, useState, useRef, useCallback, useMemo } from 'react'; import { HttpSetup } from '../../../../../src/core/public'; -import { sendRequest, SendRequestConfig, SendRequestResponse } from './send_request'; +import { + sendRequest as _sendRequest, + SendRequestConfig, + SendRequestResponse, +} from './send_request'; export interface UseRequestConfig extends SendRequestConfig { pollIntervalMs?: number; @@ -33,7 +37,7 @@ export interface UseRequestResponse { isLoading: boolean; error: E | null; data?: D | null; - sendRequest: () => Promise>; + sendRequest: () => Promise>; } export const useRequest = ( @@ -41,15 +45,15 @@ export const useRequest = ( { path, method, - query, - body, + query: _query, + body: _body, pollIntervalMs, initialData, - deserializer = (data: any): any => data, + deserializer, }: UseRequestConfig ): UseRequestResponse => { - const sendRequestRef = useRef<() => Promise>>(); - const scheduleRequestRef = useRef<() => void>(); + const isMounted = useRef(false); + const pollIntervalIdRef = useRef(null); // Main states for tracking request status and data const [error, setError] = useState(null); @@ -58,89 +62,101 @@ export const useRequest = ( // Consumers can use isInitialRequest to implement a polling UX. const [isInitialRequest, setIsInitialRequest] = useState(true); - const pollIntervalMsRef = useRef(); - const pollIntervalIdRef = useRef(null); - // We always want to use the most recently-set interval when scheduling the next request. - pollIntervalMsRef.current = pollIntervalMs; + // Convert our object to string to be able to compare them in our useMemo + // This allows the consumer to freely passed new objects to the hook on each + // render without asking him to memoize them. + const bodyToString = _body ? JSON.stringify(_body) : undefined; + const queryToString = _query ? JSON.stringify(_query) : undefined; - // Tied to every render and bound to each request. - let isOutdatedRequest = false; + const requestBody = useMemo(() => { + return { + path, + method, + query: queryToString ? JSON.parse(queryToString) : undefined, + body: bodyToString ? JSON.parse(bodyToString) : undefined, + }; + }, [path, method, queryToString, bodyToString]); - scheduleRequestRef.current = () => { - // Clear current interval + const cleanUpPollInterval = useCallback(() => { if (pollIntervalIdRef.current) { clearTimeout(pollIntervalIdRef.current); } + }, []); - // Set new interval - if (pollIntervalMsRef.current) { - pollIntervalIdRef.current = setTimeout(sendRequestRef.current!, pollIntervalMsRef.current); - } - }; + const sendRequest = useCallback(async () => { + cleanUpPollInterval(); - sendRequestRef.current = async () => { // We don't clear error or data, so it's up to the consumer to decide whether to display the // "old" error/data or loading state when a new request is in-flight. setIsLoading(true); - const requestBody = { - path, - method, - query, - body, - }; - - const response = await sendRequest(httpClient, requestBody); + const response = await _sendRequest(httpClient, requestBody); const { data: serializedResponseData, error: responseError } = response; - // If an outdated request has resolved, ignore its outdated data. - if (isOutdatedRequest) { + if (isMounted.current === false) { return { data: null, error: null }; } + setIsLoading(false); + setIsInitialRequest(false); + setError(responseError); // If there's an error, keep the data from the last request in case it's still useful to the user. if (!responseError) { - const responseData = deserializer(serializedResponseData); + const responseData = deserializer + ? deserializer(serializedResponseData) + : serializedResponseData; setData(responseData); } - setIsLoading(false); - setIsInitialRequest(false); - - // If we're on an interval, we need to schedule the next request. This also allows us to reset - // the interval if the user has manually requested the data, to avoid doubled-up requests. - scheduleRequestRef.current!(); return { data: serializedResponseData, error: responseError }; - }; + }, [requestBody, httpClient, deserializer, cleanUpPollInterval]); + + const scheduleRequest = useCallback(() => { + if (pollIntervalMs) { + pollIntervalIdRef.current = setTimeout(sendRequest, pollIntervalMs); + } + }, [pollIntervalMs, sendRequest]); + + useEffect(() => { + sendRequest(); + }, [sendRequest]); + // Whenever the scheduleRequest() changes (which changes when the pollIntervalMs changes) + // we schedule a new request useEffect(() => { - sendRequestRef.current!(); + scheduleRequest(); + return cleanUpPollInterval; + }, [scheduleRequest, cleanUpPollInterval]); + + // Whenever the data state changes (this means a previous request has fulfilled), + // we schedule a new request + useEffect(() => { + if (isMounted.current === false) { + // Don't schedule on component mount + return; + } - // 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 and the query. - }, [path, query]); + scheduleRequest(); + return cleanUpPollInterval; + }, [data, scheduleRequest, cleanUpPollInterval]); useEffect(() => { - scheduleRequestRef.current!(); + isMounted.current = true; - // Clean up timeout and mark inflight requests as stale if the poll interval changes. return () => { - /* eslint-disable-next-line react-hooks/exhaustive-deps */ - isOutdatedRequest = true; - if (pollIntervalIdRef.current) { - clearTimeout(pollIntervalIdRef.current); - } + isMounted.current = false; + // When the component unmounts, clear any existing interval. + cleanUpPollInterval(); }; - }, [pollIntervalMs]); + }, [cleanUpPollInterval]); return { isInitialRequest, isLoading, error, data, - sendRequest: sendRequestRef.current, // Gives the user the ability to manually request data + sendRequest, // Gives the user the ability to manually request data }; }; From 661a87df7fefd62a5d4fba000c08f47bd3fbc0be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien=20Loix?= Date: Tue, 4 Aug 2020 19:56:13 +0200 Subject: [PATCH 3/5] Remove unnecessary warning in comment --- .../public/request/use_request.test.ts | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/src/plugins/es_ui_shared/public/request/use_request.test.ts b/src/plugins/es_ui_shared/public/request/use_request.test.ts index 8d25136b620d22..b7c23bb3c5c65e 100644 --- a/src/plugins/es_ui_shared/public/request/use_request.test.ts +++ b/src/plugins/es_ui_shared/public/request/use_request.test.ts @@ -141,9 +141,6 @@ describe('useRequest hook', () => { expect(hookResult.isLoading).toBe(false); expect(hookResult.error).toBe(getErrorResponse().error); - // NOTE: This emits the warning "You called act(async () => ...) without await", because - // sendRequest returns a Promise. We don't want to await the resolution of this Promise, - // because we want to assert against the hook state while the Promise is pending. act(() => { hookResult.sendRequest(); }); @@ -179,9 +176,6 @@ describe('useRequest hook', () => { expect(hookResult.isLoading).toBe(false); expect(hookResult.data).toBe(getSuccessResponse().data); - // NOTE: This emits the warning "You called act(async () => ...) without await", because - // sendRequest returns a Promise. We don't want to await the resolution of this Promise, - // because we want to assert against the hook state while the Promise is pending. act(() => { hookResult.sendRequest(); }); @@ -231,10 +225,6 @@ describe('useRequest hook', () => { }); it('resets the pollIntervalMs', async () => { - // NOTE: This tests emits the warning "You called act(async () => ...) without await", because - // sendRequest returns a Promise. We don't want to await the resolution of this Promise, - // because we want to assert against the hook state while the Promise is pending. - const { setupSuccessRequest, advanceTime, hookResult, getSendRequestSpy } = helpers; const DOUBLE_REQUEST_TIME = REQUEST_TIME * 2 + 50; // We add 50 to avoid a race condition setupSuccessRequest({ pollIntervalMs: DOUBLE_REQUEST_TIME }); @@ -242,6 +232,7 @@ describe('useRequest hook', () => { // The initial request resolves, and then we'll immediately send a new one manually... await advanceTime(REQUEST_TIME); expect(getSendRequestSpy().callCount).toBe(1); + act(() => { hookResult.sendRequest(); }); @@ -260,10 +251,6 @@ describe('useRequest hook', () => { }); it(`doesn't block requests that are in flight`, async () => { - // NOTE: This test emits the warning "You called act(async () => ...) without await", because - // sendRequest returns a Promise. We don't want to await the resolution of this Promise, - // because we want to assert against the hook state while the Promise is pending. - const { setupSuccessRequest, advanceTime, From cf4059184fb033b05ea9d9dc75088adda601a2d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien=20Loix?= Date: Tue, 4 Aug 2020 20:27:56 +0200 Subject: [PATCH 4/5] Fix issue if request return an error and the scheduleRequest is not called --- .../es_ui_shared/public/request/use_request.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/plugins/es_ui_shared/public/request/use_request.ts b/src/plugins/es_ui_shared/public/request/use_request.ts index e7f99d583379af..2f596c5a4dd525 100644 --- a/src/plugins/es_ui_shared/public/request/use_request.ts +++ b/src/plugins/es_ui_shared/public/request/use_request.ts @@ -61,7 +61,8 @@ export const useRequest = ( const [data, setData] = useState(initialData); // Consumers can use isInitialRequest to implement a polling UX. - const [isInitialRequest, setIsInitialRequest] = useState(true); + const [totalRequests, setTotalRequests] = useState(0); + const isInitialRequest = totalRequests === 0; // Convert our object to string to be able to compare them in our useMemo // This allows the consumer to freely passed new objects to the hook on each @@ -99,7 +100,7 @@ export const useRequest = ( } setIsLoading(false); - setIsInitialRequest(false); + setTotalRequests((prev) => prev + 1); setError(responseError); // If there's an error, keep the data from the last request in case it's still useful to the user. @@ -114,10 +115,12 @@ export const useRequest = ( }, [requestBody, httpClient, deserializer, cleanUpPollInterval]); const scheduleRequest = useCallback(() => { + cleanUpPollInterval(); + if (pollIntervalMs) { pollIntervalIdRef.current = setTimeout(sendRequest, pollIntervalMs); } - }, [pollIntervalMs, sendRequest]); + }, [pollIntervalMs, sendRequest, cleanUpPollInterval]); useEffect(() => { sendRequest(); @@ -130,7 +133,7 @@ export const useRequest = ( return cleanUpPollInterval; }, [scheduleRequest, cleanUpPollInterval]); - // Whenever the data state changes (this means a previous request has fulfilled), + // Whenever the totalRequests state changes // we schedule a new request useEffect(() => { if (isMounted.current === false) { @@ -140,7 +143,7 @@ export const useRequest = ( scheduleRequest(); return cleanUpPollInterval; - }, [data, scheduleRequest, cleanUpPollInterval]); + }, [totalRequests, scheduleRequest, cleanUpPollInterval]); useEffect(() => { isMounted.current = true; From 34eb883f76c09895f021073081e93257ae4284e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien=20Loix?= Date: Fri, 7 Aug 2020 11:01:02 +0200 Subject: [PATCH 5/5] Remove unnecessary useEffect + fix pollIntrvalMs test --- .../public/request/use_request.test.ts | 5 +++-- .../es_ui_shared/public/request/use_request.ts | 18 ++++++------------ 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/src/plugins/es_ui_shared/public/request/use_request.test.ts b/src/plugins/es_ui_shared/public/request/use_request.test.ts index b7c23bb3c5c65e..0652b9001dd97a 100644 --- a/src/plugins/es_ui_shared/public/request/use_request.test.ts +++ b/src/plugins/es_ui_shared/public/request/use_request.test.ts @@ -47,10 +47,11 @@ describe('useRequest hook', () => { await advanceTime(REQUEST_TIME); expect(getSendRequestSpy().callCount).toBe(1); - await advanceTime(REQUEST_TIME); + // We need to advance (1) the pollIntervalMs and (2) the request time + await advanceTime(REQUEST_TIME * 2); expect(getSendRequestSpy().callCount).toBe(2); - await advanceTime(REQUEST_TIME); + await advanceTime(REQUEST_TIME * 2); expect(getSendRequestSpy().callCount).toBe(3); }); }); diff --git a/src/plugins/es_ui_shared/public/request/use_request.ts b/src/plugins/es_ui_shared/public/request/use_request.ts index 2f596c5a4dd525..3ec79f6ee77597 100644 --- a/src/plugins/es_ui_shared/public/request/use_request.ts +++ b/src/plugins/es_ui_shared/public/request/use_request.ts @@ -82,6 +82,7 @@ export const useRequest = ( const cleanUpPollInterval = useCallback(() => { if (pollIntervalIdRef.current) { clearTimeout(pollIntervalIdRef.current); + pollIntervalIdRef.current = null; } }, []); @@ -122,19 +123,14 @@ export const useRequest = ( } }, [pollIntervalMs, sendRequest, cleanUpPollInterval]); + // Send the request on component mount + // + anytime the deps of sendRequest() change useEffect(() => { sendRequest(); }, [sendRequest]); - // Whenever the scheduleRequest() changes (which changes when the pollIntervalMs changes) - // we schedule a new request - useEffect(() => { - scheduleRequest(); - return cleanUpPollInterval; - }, [scheduleRequest, cleanUpPollInterval]); - - // Whenever the totalRequests state changes - // we schedule a new request + // Schedule a new request each time a request fulfills ("totalRequests" increment) + // + anytime the deps of scheduleRequest() change useEffect(() => { if (isMounted.current === false) { // Don't schedule on component mount @@ -150,10 +146,8 @@ export const useRequest = ( return () => { isMounted.current = false; - // When the component unmounts, clear any existing interval. - cleanUpPollInterval(); }; - }, [cleanUpPollInterval]); + }, []); return { isInitialRequest,