Merge pull request #10067 from mabashian/test-warning-cleanup-3

Unit test warning cleanup

SUMMARY
These commits target the warnings that look like:
Can't perform a React state update on an unmounted component
The underlying problem here is that we have network requests that are being made by components that are subsequently being unmounted.  When the network request resolves, we attempt to update some state but the component is no longer mounted and the warning is triggered.  To address this I consolidated a lot of isMounted code into a single hook which can be used across the app to check to see whether the component in question is still mounted before attempting to update state inside of a useEffect.  This primarily applies to network requests.
I think this points to a larger issue which is that we sometimes mount components prematurely.  For example, when the job template edit component is mounted we actually mount:

JobTemplateForm (briefly)
ContentLoading
JobTemplateForm

Network requests triggered by the first mount of JobTemplateForm are suscepitble to attempting to update state on an unmounted component.  I believe this pattern exists in many places across the app but I haven't tried to solve this in this PR.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

UI

Reviewed-by: Marliana Lara <marliana.lara@gmail.com>
Reviewed-by: Kersom <None>

awx/ui_next/src/components/Lookup/MultiCredentialsLookup.jsx

@@ -12,6 +12,7 @@ import OptionsList from '../OptionsList';
 import useRequest from '../../util/useRequest';
 import { getQSConfig, parseQueryString } from '../../util/qs';
 import Lookup from './Lookup';
+import useIsMounted from '../../util/useIsMounted';
 
 const QS_CONFIG = getQSConfig('credentials', {
   page: 1,
@@ -28,6 +29,7 @@ async function loadCredentials(params, selectedCredentialTypeId) {
 function MultiCredentialsLookup(props) {
   const { value, onChange, onError, history, i18n } = props;
   const [selectedType, setSelectedType] = useState(null);
+  const isMounted = useIsMounted();
 
   const {
     result: credentialTypes,
@@ -38,8 +40,11 @@ function MultiCredentialsLookup(props) {
     useCallback(async () => {
       const types = await CredentialTypesAPI.loadAllTypes();
       const match = types.find(type => type.kind === 'ssh') || types[0];
-      setSelectedType(match);
+      if (isMounted.current) {
+        setSelectedType(match);
+      }
       return types;
+      /* eslint-disable-next-line react-hooks/exhaustive-deps */
     }, []),
     []
   );

awx/ui_next/src/components/MultiSelect/useSyncedSelectValue.js

@@ -1,4 +1,5 @@
 import { useState, useEffect } from 'react';
+import useIsMounted from '../../util/useIsMounted';
 
 /*
   Hook for using PatternFly's <Select> component when a pre-existing value
@@ -9,8 +10,12 @@ import { useState, useEffect } from 'react';
 export default function useSyncedSelectValue(value, onChange) {
   const [options, setOptions] = useState([]);
   const [selections, setSelections] = useState([]);
+  const isMounted = useIsMounted();
 
   useEffect(() => {
+    if (!isMounted.current) {
+      return;
+    }
     const newOptions = [];
     if (value !== selections && options.length) {
       const syncedValue = value.map(item => {
@@ -41,7 +46,11 @@ export default function useSyncedSelectValue(value, onChange) {
     selections: options.length ? addToStringToObjects(selections) : [],
     onSelect,
     options,
-    setOptions: newOpts => setOptions(addToStringToObjects(newOpts)),
+    setOptions: newOpts => {
+      if (isMounted.current) {
+        setOptions(addToStringToObjects(newOpts));
+      }
+    },
   };
 }
 

awx/ui_next/src/screens/Inventory/InventoryEdit/InventoryEdit.jsx

@@ -1,4 +1,4 @@
-import React, { useState, useEffect, useRef } from 'react';
+import React, { useState, useEffect } from 'react';
 import { useHistory } from 'react-router-dom';
 import { object } from 'prop-types';
 
@@ -7,6 +7,7 @@ import { InventoriesAPI, CredentialTypesAPI } from '../../../api';
 import ContentLoading from '../../../components/ContentLoading';
 import InventoryForm from '../shared/InventoryForm';
 import { getAddedAndRemoved } from '../../../util/lists';
+import useIsMounted from '../../../util/useIsMounted';
 
 function InventoryEdit({ inventory }) {
   const [error, setError] = useState(null);
@@ -14,10 +15,9 @@ function InventoryEdit({ inventory }) {
   const [contentLoading, setContentLoading] = useState(true);
   const [credentialTypeId, setCredentialTypeId] = useState(null);
   const history = useHistory();
-  const isMounted = useRef(null);
+  const isMounted = useIsMounted();
 
   useEffect(() => {
-    isMounted.current = true;
     const loadData = async () => {
       try {
         const [
@@ -47,9 +47,7 @@ function InventoryEdit({ inventory }) {
       }
     };
     loadData();
-    return () => {
-      isMounted.current = false;
-    };
+    /* eslint-disable-next-line react-hooks/exhaustive-deps */
   }, [inventory.id, contentLoading, inventory, credentialTypeId]);
 
   const handleCancel = () => {

awx/ui_next/src/screens/Inventory/InventorySourceDetail/InventorySourceDetail.jsx

@@ -1,4 +1,4 @@
-import React, { useCallback, useEffect, useRef, useState } from 'react';
+import React, { useCallback, useEffect, useState } from 'react';
 import { Link, useHistory } from 'react-router-dom';
 import { withI18n } from '@lingui/react';
 import { t } from '@lingui/macro';
@@ -23,6 +23,7 @@ import Popover from '../../../components/Popover';
 import useRequest from '../../../util/useRequest';
 import { InventorySourcesAPI } from '../../../api';
 import { relatedResourceDeleteRequests } from '../../../util/getRelatedResourceDeleteDetails';
+import useIsMounted from '../../../util/useIsMounted';
 
 function InventorySourceDetail({ inventorySource, i18n }) {
   const {
@@ -57,7 +58,7 @@ function InventorySourceDetail({ inventorySource, i18n }) {
   } = inventorySource;
   const [deletionError, setDeletionError] = useState(false);
   const history = useHistory();
-  const isMounted = useRef(null);
+  const isMounted = useIsMounted();
 
   const {
     result: sourceChoices,
@@ -75,11 +76,7 @@ function InventorySourceDetail({ inventorySource, i18n }) {
   );
 
   useEffect(() => {
-    isMounted.current = true;
     fetchSourceChoices();
-    return () => {
-      isMounted.current = false;
-    };
   }, [fetchSourceChoices]);
 
   const handleDelete = async () => {

awx/ui_next/src/screens/Job/JobOutput/JobOutput.jsx

@@ -47,6 +47,7 @@ import {
   removeParams,
   getQSConfig,
 } from '../../../util/qs';
+import useIsMounted from '../../../util/useIsMounted';
 
 const QS_CONFIG = getQSConfig('job_output', {
   order_by: 'start_line',
@@ -275,10 +276,10 @@ const cache = new CellMeasurerCache({
 function JobOutput({ job, eventRelatedSearchableKeys, eventSearchableKeys }) {
   const location = useLocation();
   const listRef = useRef(null);
-  const isMounted = useRef(false);
   const previousWidth = useRef(0);
   const jobSocketCounter = useRef(0);
   const interval = useRef(null);
+  const isMounted = useIsMounted();
   const history = useHistory();
   const [contentError, setContentError] = useState(null);
   const [cssMap, setCssMap] = useState({});
@@ -292,7 +293,6 @@ function JobOutput({ job, eventRelatedSearchableKeys, eventSearchableKeys }) {
   const [results, setResults] = useState({});
 
   useEffect(() => {
-    isMounted.current = true;
     loadJobEvents();
 
     if (isJobRunning(job.status)) {
@@ -319,7 +319,6 @@ function JobOutput({ job, eventRelatedSearchableKeys, eventSearchableKeys }) {
         ws.close();
       }
       clearInterval(interval.current);
-      isMounted.current = false;
     };
   }, [location.search]); // eslint-disable-line react-hooks/exhaustive-deps
 

awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx

@@ -45,6 +45,7 @@ import { JobTemplatesAPI } from '../../../api';
 import LabelSelect from './LabelSelect';
 import PlaybookSelect from './PlaybookSelect';
 import WebhookSubForm from './WebhookSubForm';
+import useIsMounted from '../../../util/useIsMounted';
 
 const { origin } = document.location;
 
@@ -67,6 +68,7 @@ function JobTemplateForm({
   const [enableWebhooks, setEnableWebhooks] = useState(
     Boolean(template.webhook_service)
   );
+  const isMounted = useIsMounted();
 
   const [askInventoryOnLaunchField] = useField('ask_inventory_on_launch');
   const [jobTypeField, jobTypeMeta, jobTypeHelpers] = useField({
@@ -119,8 +121,11 @@ function JobTemplateForm({
         return;
       }
       const { data } = await JobTemplatesAPI.readInstanceGroups(template.id);
-      setFieldValue('initialInstanceGroups', data.results);
-      setFieldValue('instanceGroups', [...data.results]);
+      if (isMounted.current) {
+        setFieldValue('initialInstanceGroups', data.results);
+        setFieldValue('instanceGroups', [...data.results]);
+      }
+      // eslint-disable-next-line react-hooks/exhaustive-deps
     }, [setFieldValue, template])
   );
 

awx/ui_next/src/screens/Template/shared/LabelSelect.jsx

@@ -4,8 +4,12 @@ import { Select, SelectOption, SelectVariant } from '@patternfly/react-core';
 import { t } from '@lingui/macro';
 import { LabelsAPI } from '../../../api';
 import { useSyncedSelectValue } from '../../../components/MultiSelect';
+import useIsMounted from '../../../util/useIsMounted';
 
-async function loadLabelOptions(setLabels, onError) {
+async function loadLabelOptions(setLabels, onError, isMounted) {
+  if (!isMounted.current) {
+    return;
+  }
   let labels;
   try {
     const { data } = await LabelsAPI.read({
@@ -32,11 +36,12 @@ async function loadLabelOptions(setLabels, onError) {
 
 function LabelSelect({ value, placeholder, onChange, onError, createText }) {
   const [isLoading, setIsLoading] = useState(true);
+  const [isExpanded, setIsExpanded] = useState(false);
+  const isMounted = useIsMounted();
   const { selections, onSelect, options, setOptions } = useSyncedSelectValue(
     value,
     onChange
   );
-  const [isExpanded, setIsExpanded] = useState(false);
 
   const toggleExpanded = toggleValue => {
     setIsExpanded(toggleValue);
@@ -44,7 +49,10 @@ function LabelSelect({ value, placeholder, onChange, onError, createText }) {
 
   useEffect(() => {
     (async () => {
-      await loadLabelOptions(setOptions, onError);
+      await loadLabelOptions(setOptions, onError, isMounted);
+      if (!isMounted.current) {
+        return;
+      }
       setIsLoading(false);
     })();
     /* eslint-disable-next-line react-hooks/exhaustive-deps */

awx/ui_next/src/util/useIsMounted.js

@@ -0,0 +1,12 @@
+import { useEffect, useRef } from 'react';
+
+export default function useIsMounted() {
+  const isMounted = useRef(null);
+  useEffect(() => {
+    isMounted.current = true;
+    return () => {
+      isMounted.current = false;
+    };
+  });
+  return isMounted;
+}

awx/ui_next/src/util/useRequest.js

@@ -1,10 +1,11 @@
-import { useEffect, useState, useRef, useCallback } from 'react';
+import { useEffect, useState, useCallback } from 'react';
 import { useLocation, useHistory } from 'react-router-dom';
 import {
   parseQueryString,
   replaceParams,
   encodeNonDefaultQueryString,
 } from './qs';
+import useIsMounted from './useIsMounted';
 
 /*
  * The useRequest hook accepts a request function and returns an object with
@@ -22,14 +23,7 @@ export default function useRequest(makeRequest, initialValue) {
   const [result, setResult] = useState(initialValue);
   const [error, setError] = useState(null);
   const [isLoading, setIsLoading] = useState(false);
-  const isMounted = useRef(null);
-
-  useEffect(() => {
-    isMounted.current = true;
-    return () => {
-      isMounted.current = false;
-    };
-  }, []);
+  const isMounted = useIsMounted();
 
   return {
     result,