From 6c9913c9d39f09a05382b5d822398c31bc2ab5af Mon Sep 17 00:00:00 2001
From: Viktor Bergehall <v-vbergehall@microsoft.com>
Date: Fri, 24 Mar 2023 09:24:53 +0100
Subject: [PATCH] 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 <paulo@ragonha.me>
---
 .../core/src/hooks/useFacetCallback.spec.tsx  | 32 +++++++++++++++++--
 .../core/src/hooks/useFacetCallback.ts        | 12 ++++---
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/packages/@react-facet/core/src/hooks/useFacetCallback.spec.tsx b/packages/@react-facet/core/src/hooks/useFacetCallback.spec.tsx
index bb564e09..741c8728 100644
--- 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
index 62adb757..22cea614 100644
--- 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],
   )
 }