From c53e15dd780fdf108b9b35141f9f7f47b540fb79 Mon Sep 17 00:00:00 2001 From: Samy Ouyahia <103439265+souyahia-monk@users.noreply.github.com> Date: Tue, 19 Nov 2024 14:19:08 +0100 Subject: [PATCH] Removed memory leaks in MonkAppStateProvider and LiveConfigAppProvider components (#882) * Added useIsMounted hook * Removed memory leaks in MonkAppStateProvider and LiveConfigAppProvider components --- .../src/__mocks__/@monkvision/common.tsx | 1 + .../LiveConfigAppProvider.tsx | 41 ++++++++++--------- .../components/LiveConfigAppProvider.test.tsx | 7 +++- packages/common/README/HOOKS.md | 20 +++++++++ packages/common/src/apps/appStateProvider.tsx | 6 ++- packages/common/src/hooks/useIsMounted.ts | 19 +++++++++ .../common/test/hooks/useIsMounted.test.ts | 11 +++++ 7 files changed, 82 insertions(+), 23 deletions(-) create mode 100644 packages/common/src/hooks/useIsMounted.ts create mode 100644 packages/common/test/hooks/useIsMounted.test.ts diff --git a/configs/test-utils/src/__mocks__/@monkvision/common.tsx b/configs/test-utils/src/__mocks__/@monkvision/common.tsx index 66164dfd1..72b341851 100644 --- a/configs/test-utils/src/__mocks__/@monkvision/common.tsx +++ b/configs/test-utils/src/__mocks__/@monkvision/common.tsx @@ -136,4 +136,5 @@ export = { isInputErrorDisplayed: jest.fn(() => false), isInputTouchedOrDirty: jest.fn(() => false), })), + useIsMounted: jest.fn(() => jest.fn(() => true)), }; diff --git a/packages/common-ui-web/src/components/LiveConfigAppProvider/LiveConfigAppProvider.tsx b/packages/common-ui-web/src/components/LiveConfigAppProvider/LiveConfigAppProvider.tsx index 03b63397f..c9cfb3e9e 100644 --- a/packages/common-ui-web/src/components/LiveConfigAppProvider/LiveConfigAppProvider.tsx +++ b/packages/common-ui-web/src/components/LiveConfigAppProvider/LiveConfigAppProvider.tsx @@ -1,10 +1,11 @@ import { MonkAppStateProvider, MonkAppStateProviderProps, + useAsyncEffect, useI18nSync, useLoadingState, } from '@monkvision/common'; -import { PropsWithChildren, useCallback, useEffect, useState } from 'react'; +import { PropsWithChildren, useState } from 'react'; import { CaptureAppConfig } from '@monkvision/types'; import { MonkApi } from '@monkvision/network'; import { useMonitoring } from '@monkvision/monitoring'; @@ -50,29 +51,29 @@ export function LiveConfigAppProvider({ const loading = useLoadingState(true); const [config, setConfig] = useState(null); const { handleError } = useMonitoring(); + const [retry, setRetry] = useState(0); - const fetchLiveConfig = useCallback(() => { - if (localConfig) { - loading.onSuccess(); - setConfig(localConfig); - return; - } - loading.start(); - setConfig(null); - MonkApi.getLiveConfig(id) - .then((result) => { + useAsyncEffect( + () => { + if (localConfig) { + return Promise.resolve(localConfig); + } + loading.start(); + setConfig(null); + return MonkApi.getLiveConfig(id); + }, + [id, localConfig, retry], + { + onResolve: (result) => { loading.onSuccess(); setConfig(result); - }) - .catch((err) => { + }, + onReject: (err) => { handleError(err); loading.onError(); - }); - }, [id, localConfig]); - - useEffect(() => { - fetchLiveConfig(); - }, [fetchLiveConfig]); + }, + }, + ); if (loading.isLoading || loading.error || !config) { return ( @@ -83,7 +84,7 @@ export function LiveConfigAppProvider({
Unable to fetch application configuration. Please try again in a few minutes.
- diff --git a/packages/common-ui-web/test/components/LiveConfigAppProvider.test.tsx b/packages/common-ui-web/test/components/LiveConfigAppProvider.test.tsx index 9d0daccd1..f8f472240 100644 --- a/packages/common-ui-web/test/components/LiveConfigAppProvider.test.tsx +++ b/packages/common-ui-web/test/components/LiveConfigAppProvider.test.tsx @@ -6,14 +6,19 @@ jest.mock('../../src/components/Button', () => ({ jest.mock('../../src/components/Spinner', () => ({ Spinner: jest.fn(() => <>), })); +const { useAsyncEffect: useAsyncEffectActual } = jest.requireActual('@monkvision/common'); import { act, render, screen, waitFor } from '@testing-library/react'; import { createFakePromise, expectPropsOnChildMock } from '@monkvision/test-utils'; -import { MonkAppStateProvider, useLoadingState } from '@monkvision/common'; +import { MonkAppStateProvider, useAsyncEffect, useLoadingState } from '@monkvision/common'; import { MonkApi } from '@monkvision/network'; import { Button, LiveConfigAppProvider, Spinner } from '../../src'; describe('LiveConfigAppProvider component', () => { + beforeEach(() => { + (useAsyncEffect as jest.Mock).mockImplementation(useAsyncEffectActual); + }); + afterEach(() => { jest.clearAllMocks(); }); diff --git a/packages/common/README/HOOKS.md b/packages/common/README/HOOKS.md index cadbb5060..77399f97f 100644 --- a/packages/common/README/HOOKS.md +++ b/packages/common/README/HOOKS.md @@ -83,6 +83,26 @@ function TestComponent() { This custom hook creates an interval that calls the provided callback every `delay` milliseconds. If `delay` is `null` or less than 0, the callback will not be called. +### useIsMounted +```tsx +import { useIsMounted } from '@monkvision/common'; + +function TestComponent() { + const [example, setExample] = useState(0); + const isMounted = useIsMounted(); + + useEffect(() => { + myAsyncFunc().then((value) => { + if (isMounted()) { + setExample(value); + } + }).catch(console.error); + }, [isMounted]); +} +``` +Custom hook returning a ref to a util function returning `true` if the component using the hook is mounted, and false +otherwise. Can be used to cancel asynchronous calls on component unmounts. + ### useForm ```tsx diff --git a/packages/common/src/apps/appStateProvider.tsx b/packages/common/src/apps/appStateProvider.tsx index 1be1803d2..64285442c 100644 --- a/packages/common/src/apps/appStateProvider.tsx +++ b/packages/common/src/apps/appStateProvider.tsx @@ -14,6 +14,7 @@ import { MonkAppState, MonkAppStateContext } from './appState'; import { useAppStateMonitoring } from './monitoring'; import { useAppStateAnalytics } from './analytics'; import { getAvailableVehicleTypes } from '../utils'; +import { useIsMounted } from '../hooks/useIsMounted'; /** * Local storage key used within Monk web applications to store the authentication token. @@ -87,6 +88,7 @@ export function MonkAppStateProvider({ const [steeringWheel, setSteeringWheel] = useState(null); const availableVehicleTypes = useMemo(() => getAvailableVehicleTypes(config), [config]); const monkSearchParams = useMonkSearchParams({ availableVehicleTypes }); + const isMounted = useIsMounted(); useAppStateMonitoring({ authToken, inspectionId, vehicleType, steeringWheel }); useAppStateAnalytics({ inspectionId }); @@ -100,12 +102,12 @@ export function MonkAppStateProvider({ setVehicleType((param) => monkSearchParams.get(MonkSearchParam.VEHICLE_TYPE) ?? param); setSteeringWheel((param) => monkSearchParams.get(MonkSearchParam.STEERING_WHEEL) ?? param); const lang = monkSearchParams.get(MonkSearchParam.LANGUAGE); - if (lang) { + if (lang && isMounted()) { onFetchLanguage?.(lang); } } - if (fetchedToken) { + if (fetchedToken && isMounted()) { setAuthToken(fetchedToken); onFetchAuthToken?.(); } diff --git a/packages/common/src/hooks/useIsMounted.ts b/packages/common/src/hooks/useIsMounted.ts new file mode 100644 index 000000000..561ceea7f --- /dev/null +++ b/packages/common/src/hooks/useIsMounted.ts @@ -0,0 +1,19 @@ +import { useCallback, useEffect, useRef } from 'react'; + +/** + * Custom hook returning a ref to a util function returning `true` if the component using the hook is mounted, and false + * otherwise. + */ +export function useIsMounted(): () => boolean { + const isMounted = useRef(false); + + useEffect(() => { + isMounted.current = true; + + return () => { + isMounted.current = false; + }; + }, []); + + return useCallback(() => isMounted.current, []); +} diff --git a/packages/common/test/hooks/useIsMounted.test.ts b/packages/common/test/hooks/useIsMounted.test.ts new file mode 100644 index 000000000..90450a6df --- /dev/null +++ b/packages/common/test/hooks/useIsMounted.test.ts @@ -0,0 +1,11 @@ +import { useIsMounted } from '../../src/hooks/useIsMounted'; +import { renderHook } from '@testing-library/react-hooks'; + +describe('useIsMounted hook', () => { + it('should return true when the component is mounted and false when unmounted', () => { + const { result, unmount } = renderHook(useIsMounted); + expect(result.current()).toBe(true); + unmount(); + expect(result.current()).toBe(false); + }); +});