Keep useFacetCallback instance same even if facets instances change (#…

…119) * WIP: Make sure we keep the same callback instance when useFacetCallback gets re-rendered * Added unit test * Updated unit test * Fixed import error --------- Co-authored-by: Paulo Ragonha <[email protected]>
Mojang · Mar 24, 2023 · 6c9913c · 6c9913c
1 parent 04f6fd5
commit 6c9913c
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 6 deletions.
diff --git a/packages/@react-facet/core/src/hooks/useFacetCallback.spec.tsx b/packages/@react-facet/core/src/hooks/useFacetCallback.spec.tsx
@@ -3,8 +3,8 @@ import { act, render } from '@react-facet/dom-fiber-testing-library'
 import { useFacetCallback } from './useFacetCallback'
 import { useFacetEffect } from './useFacetEffect'
 import { useFacetMap } from './useFacetMap'
-import { NO_VALUE } from '../types'
-import { createFacet } from '../facet'
+import { NO_VALUE, Facet } from '../types'
+import { createFacet, createStaticFacet } from '../facet'
 import { NoValue } from '..'
 
 it('captures the current value of the facet in a function that can be used as handler', () => {
@@ -321,4 +321,32 @@ describe('regressions', () => {
       expect(result).toBe('valuestring')
     })
   })
+
+  it('always returns the same callback instance, even if the Facet instances change', () => {
+    let handler: () => void = () => {}
+    const facetA = createStaticFacet('a')
+    const facetB = createStaticFacet('b')
+
+    const TestComponent = ({ facet }: { facet: Facet<string> }) => {
+      handler = useFacetCallback(
+        (a) => () => {
+          return a
+        },
+        [],
+        [facet],
+      )
+
+      return null
+    }
+
+    const { rerender } = render(<TestComponent facet={facetA} />)
+    const firstHandler = handler
+    expect(firstHandler()).toBe('a')
+
+    rerender(<TestComponent facet={facetB} />)
+    const secondHandler = handler
+    expect(secondHandler()).toBe('b')
+
+    expect(firstHandler).toBe(secondHandler)
+  })
 })
diff --git a/packages/@react-facet/core/src/hooks/useFacetCallback.ts b/packages/@react-facet/core/src/hooks/useFacetCallback.ts
@@ -1,4 +1,4 @@
-import { useCallback, useLayoutEffect } from 'react'
+import { useCallback, useLayoutEffect, useRef } from 'react'
 import { Facet, NO_VALUE, ExtractFacetValues, NoValue } from '../types'
 
 /**
@@ -59,8 +59,14 @@ export function useFacetCallback<M, Y extends Facet<unknown>[], T extends [...Y]
   // eslint-disable-next-line react-hooks/exhaustive-deps
   const callbackMemoized = useCallback(callback, dependencies)
 
+  // Setup a ref so that the callback instance below can be kept the same
+  // when Facet instances change across re-renders
+  const facetsRef = useRef(facets)
+  facetsRef.current = facets
+
   return useCallback(
     (...args: K) => {
+      const facets = facetsRef.current
       const values = facets.map((facet) => facet.get())
 
       for (const value of values) {
@@ -69,8 +75,6 @@ export function useFacetCallback<M, Y extends Facet<unknown>[], T extends [...Y]
 
       return callbackMemoized(...(values as ExtractFacetValues<T>))(...(args as K))
     },
-    // We care about each individual facet and if any is a different reference
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-    [callbackMemoized, defaultReturnValue, ...facets],
+    [callbackMemoized, defaultReturnValue],
   )
 }