Use request cr changes

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(<TestComponent requestConfig={config} />);
+      element = mount(<TestComponent requestConfig={config} onHookChange={onChange} />);
     });
   };
 

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);
       });
     });
@@ -141,9 +142,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 +177,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,17 +226,14 @@ 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;
+        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...
         await advanceTime(REQUEST_TIME);
         expect(getSendRequestSpy().callCount).toBe(1);
+
         act(() => {
           hookResult.sendRequest();
         });
@@ -253,17 +245,13 @@ 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);
       });
 
       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,

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,114 +37,123 @@ export interface UseRequestResponse<D = any, E = Error> {
   isLoading: boolean;
   error: E | null;
   data?: D | null;
-  sendRequest: () => Promise<SendRequestResponse<D, E>>;
+  sendRequest: () => Promise<SendRequestResponse<D | null, E | null>>;
 }
 
 export const useRequest = <D = any, E = Error>(
   httpClient: HttpSetup,
   {
     path,
     method,
-    query,
-    body,
+    query: _query,
+    body: _body,
     pollIntervalMs,
     initialData,
-    deserializer = (data: any): any => data,
+    deserializer,
   }: UseRequestConfig
 ): UseRequestResponse<D, E> => {
-  const sendRequestRef = useRef<() => Promise<SendRequestResponse<D, E>>>();
-  const scheduleRequestRef = useRef<() => void>();
+  const isMounted = useRef(false);
+  const pollIntervalIdRef = useRef<any>(null);
 
   // Main states for tracking request status and data
   const [error, setError] = useState<null | any>(null);
   const [isLoading, setIsLoading] = useState<boolean>(true);
   const [data, setData] = useState<any>(initialData);
 
   // Consumers can use isInitialRequest to implement a polling UX.
-  const [isInitialRequest, setIsInitialRequest] = useState<boolean>(true);
-  const pollIntervalMsRef = useRef<number | undefined>();
-  const pollIntervalIdRef = useRef<any>(null);
+  const [totalRequests, setTotalRequests] = useState(0);
+  const isInitialRequest = totalRequests === 0;
 
-  // 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);
+      pollIntervalIdRef.current = null;
     }
+  }, []);
 
-    // 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<D, E>(httpClient, requestBody);
+    const response = await _sendRequest<D, E>(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);
+    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.
     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(() => {
+    cleanUpPollInterval();
 
+    if (pollIntervalMs) {
+      pollIntervalIdRef.current = setTimeout(sendRequest, pollIntervalMs);
+    }
+  }, [pollIntervalMs, sendRequest, cleanUpPollInterval]);
+
+  // Send the request on component mount
+  // + anytime the deps of sendRequest() change
   useEffect(() => {
-    sendRequestRef.current!();
+    sendRequest();
+  }, [sendRequest]);
+
+  // 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
+      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;
+  }, [totalRequests, 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;
     };
-  }, [pollIntervalMs]);
+  }, []);
 
   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
   };
 };