fix(cacheable-section): stable references to avoid loops [LIBS-642] (#…

…1385)

* fix(cacheable-sections): stable references

* refactor: avoid needing eslint disable

* chore: consistent test names

services/offline/src/lib/__tests__/cacheable-section-state.test.tsx

@@ -0,0 +1,45 @@
+import { renderHook } from '@testing-library/react-hooks'
+import React, { FC } from 'react'
+import { mockOfflineInterface } from '../../utils/test-mocks'
+import { useCachedSection, useRecordingState } from '../cacheable-section-state'
+import { OfflineProvider } from '../offline-provider'
+
+const wrapper: FC = ({ children }) => (
+    <OfflineProvider offlineInterface={mockOfflineInterface}>
+        {children}
+    </OfflineProvider>
+)
+
+test('useRecordingState has stable references', () => {
+    const { result, rerender } = renderHook(() => useRecordingState('one'), {
+        wrapper,
+    })
+
+    const origRecordingState = result.current.recordingState
+    const origSetRecordingState = result.current.setRecordingState
+    const origRemoveRecordingState = result.current.removeRecordingState
+
+    rerender()
+
+    expect(result.current.recordingState).toBe(origRecordingState)
+    expect(result.current.setRecordingState).toBe(origSetRecordingState)
+    expect(result.current.removeRecordingState).toBe(origRemoveRecordingState)
+})
+
+test('useCachedSection has stable references', () => {
+    const { result, rerender } = renderHook(() => useCachedSection('one'), {
+        wrapper,
+    })
+
+    const origIsCached = result.current.isCached
+    const origLastUpdated = result.current.lastUpdated
+    const origRemove = result.current.remove
+    const origSyncCachedSections = result.current.syncCachedSections
+
+    rerender()
+
+    expect(result.current.isCached).toBe(origIsCached)
+    expect(result.current.lastUpdated).toBe(origLastUpdated)
+    expect(result.current.remove).toBe(origRemove)
+    expect(result.current.syncCachedSections).toBe(origSyncCachedSections)
+})

services/offline/src/lib/__tests__/use-cacheable-section.test.tsx

@@ -41,6 +41,31 @@ it.skip('renders in the default state initially', () => {
     expect(result.current.lastUpdated).toBeUndefined()
 })
 
+it('has stable references', () => {
+    const wrapper: FC = ({ children }) => (
+        <OfflineProvider offlineInterface={mockOfflineInterface}>
+            {children}
+        </OfflineProvider>
+    )
+    const { result, rerender } = renderHook(() => useCacheableSection('one'), {
+        wrapper,
+    })
+
+    const origRecordingState = result.current.recordingState
+    const origStartRecording = result.current.startRecording
+    const origLastUpdated = result.current.lastUpdated
+    const origIsCached = result.current.isCached
+    const origRemove = result.current.remove
+
+    rerender()
+
+    expect(result.current.recordingState).toBe(origRecordingState)
+    expect(result.current.startRecording).toBe(origStartRecording)
+    expect(result.current.lastUpdated).toBe(origLastUpdated)
+    expect(result.current.isCached).toBe(origIsCached)
+    expect(result.current.remove).toBe(origRemove)
+})
+
 it.skip('handles a successful recording', async (done) => {
     const [sectionId, timeoutDelay] = ['one', 1234]
     const testOfflineInterface = {

services/offline/src/lib/cacheable-section-state.tsx

@@ -1,5 +1,5 @@
 import PropTypes from 'prop-types'
-import React from 'react'
+import React, { useEffect, useCallback, useMemo } from 'react'
 import {
     GlobalStateStore,
     IndexedDBCachedSection,
@@ -76,7 +76,7 @@ export function CacheableSectionProvider({
     const store = useConst(createCacheableSectionStore)
 
     // On load, get sections and add to store
-    React.useEffect(() => {
+    useEffect(() => {
         if (offlineInterface) {
             offlineInterface.getCachedSections().then((sections) => {
                 store.mutate((state) => ({
@@ -107,20 +107,43 @@ interface RecordingStateControls {
  * @returns {Object} { recordingState: String, setRecordingState: Function, removeRecordingState: Function}
  */
 export function useRecordingState(id: string): RecordingStateControls {
-    const [recordingState] = useGlobalState(
-        (state) => state.recordingStates[id]
+    const recordingStateSelector = useCallback(
+        (state) => state.recordingStates[id],
+        [id]
+    )
+    const [recordingState] = useGlobalState(recordingStateSelector)
+
+    const setRecordingStateMutationCreator = useCallback(
+        (newState) => (state: any) => ({
+            ...state,
+            recordingStates: { ...state.recordingStates, [id]: newState },
+        }),
+        [id]
+    )
+    const setRecordingState = useGlobalStateMutation(
+        setRecordingStateMutationCreator
+    )
+
+    const removeRecordingStateMutationCreator = useCallback(
+        () => (state: any) => {
+            const recordingStates = { ...state.recordingStates }
+            delete recordingStates[id]
+            return { ...state, recordingStates }
+        },
+        [id]
+    )
+    const removeRecordingState = useGlobalStateMutation(
+        removeRecordingStateMutationCreator
+    )
+
+    return useMemo(
+        () => ({
+            recordingState,
+            setRecordingState,
+            removeRecordingState,
+        }),
+        [recordingState, setRecordingState, removeRecordingState]
     )
-    const setRecordingState = useGlobalStateMutation((newState) => (state) => ({
-        ...state,
-        recordingStates: { ...state.recordingStates, [id]: newState },
-    }))
-    const removeRecordingState = useGlobalStateMutation(() => (state) => {
-        const recordingStates = { ...state.recordingStates }
-        delete recordingStates[id]
-        return { ...state, recordingStates }
-    })
-
-    return { recordingState, setRecordingState, removeRecordingState }
 }
 
 /**
@@ -131,17 +154,22 @@ export function useRecordingState(id: string): RecordingStateControls {
  */
 function useSyncCachedSections() {
     const offlineInterface = useOfflineInterface()
-    const setCachedSections = useGlobalStateMutation(
-        (cachedSections) => (state) => ({
+
+    const setCachedSectionsMutationCreator = useCallback(
+        (cachedSections) => (state: any) => ({
             ...state,
             cachedSections,
-        })
+        }),
+        []
+    )
+    const setCachedSections = useGlobalStateMutation(
+        setCachedSectionsMutationCreator
     )
 
-    return async function syncCachedSections() {
+    return useCallback(async () => {
         const sections = await offlineInterface.getCachedSections()
         setCachedSections(getSectionsById(sections))
-    }
+    }, [offlineInterface, setCachedSections])
 }
 
 interface CachedSectionsControls {
@@ -167,19 +195,25 @@ export function useCachedSections(): CachedSectionsControls {
      * Returns a promise that resolves to `true` if a section is found and
      * deleted, or `false` if asection with the specified ID does not exist.
      */
-    async function removeById(id: string) {
-        const success = await offlineInterface.removeSection(id)
-        if (success) {
-            await syncCachedSections()
-        }
-        return success
-    }
+    const removeById = useCallback(
+        async (id: string) => {
+            const success = await offlineInterface.removeSection(id)
+            if (success) {
+                await syncCachedSections()
+            }
+            return success
+        },
+        [offlineInterface, syncCachedSections]
+    )
 
-    return {
-        cachedSections,
-        removeById,
-        syncCachedSections,
-    }
+    return useMemo(
+        () => ({
+            cachedSections,
+            removeById,
+            syncCachedSections,
+        }),
+        [cachedSections, removeById, syncCachedSections]
+    )
 }
 
 interface CachedSectionControls {
@@ -210,18 +244,21 @@ export function useCachedSection(id: string): CachedSectionControls {
      * Returns `true` if a section is found and deleted, or `false` if a
      * section with the specified ID does not exist.
      */
-    async function remove() {
+    const remove = useCallback(async () => {
         const success = await offlineInterface.removeSection(id)
         if (success) {
             await syncCachedSections()
         }
         return success
-    }
+    }, [offlineInterface, id, syncCachedSections])
 
-    return {
-        lastUpdated,
-        isCached: !!lastUpdated,
-        remove,
-        syncCachedSections,
-    }
+    return useMemo(
+        () => ({
+            lastUpdated,
+            isCached: !!lastUpdated,
+            remove,
+            syncCachedSections,
+        }),
+        [lastUpdated, remove, syncCachedSections]
+    )
 }

services/offline/src/lib/cacheable-section.tsx

@@ -1,5 +1,5 @@
 import PropTypes from 'prop-types'
-import React, { useEffect } from 'react'
+import React, { useCallback, useEffect, useMemo } from 'react'
 import { RecordingState } from '../types'
 import { useRecordingState, useCachedSection } from './cacheable-section-state'
 import { useOfflineInterface } from './offline-interface'
@@ -63,58 +63,74 @@ export function useCacheableSection(id: string): CacheableSectionControls {
         }
     }, []) // eslint-disable-line react-hooks/exhaustive-deps
 
-    function startRecording({
-        recordingTimeoutDelay = 1000,
-        onStarted,
-        onCompleted,
-        onError,
-    }: CacheableSectionStartRecordingOptions = {}) {
-        // This promise resolving means that the message to the service worker
-        // to start recording was successful. Waiting for resolution prevents
-        // unnecessarily rerendering the whole component in case of an error
-        return offlineInterface
-            .startRecording({
-                sectionId: id,
-                recordingTimeoutDelay,
-                onStarted: () => {
-                    onRecordingStarted()
-                    onStarted && onStarted()
-                },
-                onCompleted: () => {
-                    onRecordingCompleted()
-                    onCompleted && onCompleted()
-                },
-                onError: (error) => {
-                    onRecordingError(error)
-                    onError && onError(error)
-                },
-            })
-            .then(() => setRecordingState(recordingStates.pending))
-    }
-
-    function onRecordingStarted() {
+    const onRecordingStarted = useCallback(() => {
         setRecordingState(recordingStates.recording)
-    }
+    }, [setRecordingState])
 
-    function onRecordingCompleted() {
+    const onRecordingCompleted = useCallback(() => {
         setRecordingState(recordingStates.default)
         syncCachedSections()
-    }
+    }, [setRecordingState, syncCachedSections])
+
+    const onRecordingError = useCallback(
+        (error: Error) => {
+            console.error('Error during recording:', error)
+            setRecordingState(recordingStates.error)
+        },
+        [setRecordingState]
+    )
 
-    function onRecordingError(error: Error) {
-        console.error('Error during recording:', error)
-        setRecordingState(recordingStates.error)
-    }
+    const startRecording = useCallback(
+        ({
+            recordingTimeoutDelay = 1000,
+            onStarted,
+            onCompleted,
+            onError,
+        }: CacheableSectionStartRecordingOptions = {}) => {
+            // This promise resolving means that the message to the service worker
+            // to start recording was successful. Waiting for resolution prevents
+            // unnecessarily rerendering the whole component in case of an error
+            return offlineInterface
+                .startRecording({
+                    sectionId: id,
+                    recordingTimeoutDelay,
+                    onStarted: () => {
+                        onRecordingStarted()
+                        onStarted && onStarted()
+                    },
+                    onCompleted: () => {
+                        onRecordingCompleted()
+                        onCompleted && onCompleted()
+                    },
+                    onError: (error) => {
+                        onRecordingError(error)
+                        onError && onError(error)
+                    },
+                })
+                .then(() => setRecordingState(recordingStates.pending))
+        },
+        [
+            id,
+            offlineInterface,
+            onRecordingCompleted,
+            onRecordingError,
+            onRecordingStarted,
+            setRecordingState,
+        ]
+    )
 
     // isCached, lastUpdated, remove: _could_ be accessed by useCachedSection,
     // but provided through this hook for convenience
-    return {
-        recordingState,
-        startRecording,
-        lastUpdated,
-        isCached,
-        remove,
-    }
+    return useMemo(
+        () => ({
+            recordingState,
+            startRecording,
+            lastUpdated,
+            isCached,
+            remove,
+        }),
+        [recordingState, startRecording, lastUpdated, isCached, remove]
+    )
 }
 
 interface CacheableSectionProps {