From b3130664e68b3eac5a0bc89a7a55f1f31d7f92c1 Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Thu, 2 Mar 2023 11:23:19 +0100 Subject: [PATCH 1/4] Fix useFacetCallback having outdated values --- .../core/src/hooks/useFacetCallback.spec.tsx | 41 +++++++++++++++++-- .../core/src/hooks/useFacetCallback.ts | 16 ++++---- 2 files changed, 44 insertions(+), 13 deletions(-) diff --git a/packages/@react-facet/core/src/hooks/useFacetCallback.spec.tsx b/packages/@react-facet/core/src/hooks/useFacetCallback.spec.tsx index 59b7ed7b..814d92b6 100644 --- a/packages/@react-facet/core/src/hooks/useFacetCallback.spec.tsx +++ b/packages/@react-facet/core/src/hooks/useFacetCallback.spec.tsx @@ -181,8 +181,10 @@ it('returns NO_VALUE if any facet has NO_VALUE and skip calling the callback', ( it('has proper return type with NO_VALUE in it', () => { const facetA = createFacet({ initialValue: 'a' }) + let handler: (event: string) => void + const TestComponent = () => { - const handler = useFacetCallback( + handler = useFacetCallback( (a) => (b: string) => { return a + b }, @@ -190,13 +192,15 @@ it('has proper return type with NO_VALUE in it', () => { [facetA], ) - if (handler('string') !== NO_VALUE) { - throw new Error('Expected NO_VALUE') - } return null } render() + + act(() => { + const result = handler('string') + expect(result).toBe('astring') + }) }) it('returns the defaultValue, when provided, if any facet has NO_VALUE and skip calling the callback', () => { @@ -229,3 +233,32 @@ it('returns the defaultValue, when provided, if any facet has NO_VALUE and skip expect(callback).not.toHaveBeenCalledWith() }) + +it('should always have the current value of tracked facets', () => { + const facetA = createFacet({ initialValue: NO_VALUE }) + + let handler: (event: string) => void = () => {} + + const TestComponent = () => { + handler = useFacetCallback( + (a) => (b: string) => { + return a + b + }, + [], + [facetA], + ) + + return null + } + + facetA.observe(() => { + const result = handler('string') + expect(result).toBe('newstring') + }) + + render() + + act(() => { + facetA.set('new') + }) +}) diff --git a/packages/@react-facet/core/src/hooks/useFacetCallback.ts b/packages/@react-facet/core/src/hooks/useFacetCallback.ts index f73d8987..6971a7a6 100644 --- a/packages/@react-facet/core/src/hooks/useFacetCallback.ts +++ b/packages/@react-facet/core/src/hooks/useFacetCallback.ts @@ -43,14 +43,10 @@ export function useFacetCallback[], T extends [...Y] facets: T, defaultReturnValue?: M, ): (...args: K) => M | NoValue { - const facetsRef = useRef[]>(facets.map(() => NO_VALUE)) - useLayoutEffect(() => { - const unsubscribes = facets.map((facet, index) => { - return facet.observe((value) => { - facetsRef.current[index] = value - }) - }) + // Make sure to start subscriptions, even though we are getting the values directly from them + // We read the values using `.get` to make sure they are always up-to-date + const unsubscribes = facets.map((facet) => facet.observe(noop)) return () => { unsubscribes.forEach((unsubscribe) => unsubscribe()) @@ -66,7 +62,7 @@ export function useFacetCallback[], T extends [...Y] // eslint-disable-next-line react-hooks/exhaustive-deps return useCallback( (...args: K) => { - const values = facetsRef.current + const values = facets.map((facet) => facet.get()) for (const value of values) { if (value === NO_VALUE) return defaultReturnValue != null ? defaultReturnValue : NO_VALUE @@ -74,6 +70,8 @@ export function useFacetCallback[], T extends [...Y] return callbackMemoized(...(values as ExtractFacetValues))(...(args as K)) }, - [callbackMemoized, facetsRef, defaultReturnValue], + [callbackMemoized, defaultReturnValue, ...facets], ) } + +const noop = () => {} From 295477b7c87c094713d96bfa0cd6d798f1caa523 Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Thu, 2 Mar 2023 13:53:39 +0100 Subject: [PATCH 2/4] Remove unused imports --- packages/@react-facet/core/src/hooks/useFacetCallback.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@react-facet/core/src/hooks/useFacetCallback.ts b/packages/@react-facet/core/src/hooks/useFacetCallback.ts index 6971a7a6..92a3541a 100644 --- a/packages/@react-facet/core/src/hooks/useFacetCallback.ts +++ b/packages/@react-facet/core/src/hooks/useFacetCallback.ts @@ -1,5 +1,5 @@ -import { useCallback, useLayoutEffect, useRef } from 'react' -import { Facet, NO_VALUE, Option, ExtractFacetValues, NoValue } from '../types' +import { useCallback, useLayoutEffect } from 'react' +import { Facet, NO_VALUE, ExtractFacetValues, NoValue } from '../types' /** * Creates a callback that depends on the value of a facet. From 3cc7d9d71d413d9998495c0ca425405e283c0f1d Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Thu, 2 Mar 2023 13:58:46 +0100 Subject: [PATCH 3/4] Better document the test --- .../@react-facet/core/src/hooks/useFacetCallback.spec.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/@react-facet/core/src/hooks/useFacetCallback.spec.tsx b/packages/@react-facet/core/src/hooks/useFacetCallback.spec.tsx index 814d92b6..922a1325 100644 --- a/packages/@react-facet/core/src/hooks/useFacetCallback.spec.tsx +++ b/packages/@react-facet/core/src/hooks/useFacetCallback.spec.tsx @@ -251,6 +251,8 @@ it('should always have the current value of tracked facets', () => { return null } + // We make sure to be the first listener registered, so this is called before + // the listener within the useFacetCallback (which would have created the issue) facetA.observe(() => { const result = handler('string') expect(result).toBe('newstring') @@ -258,6 +260,8 @@ it('should always have the current value of tracked facets', () => { render() + // In this act, the effect within useFacetCallback will be executed, subscribing for changes of the facetA + // Then we set the value, causing the listener above to being called act(() => { facetA.set('new') }) From e9b7f679f1fcd76c51c0eb1bc25d5b914b1a8426 Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Thu, 2 Mar 2023 14:15:55 +0100 Subject: [PATCH 4/4] Fix linting error --- packages/@react-facet/core/src/hooks/useFacetCallback.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/@react-facet/core/src/hooks/useFacetCallback.ts b/packages/@react-facet/core/src/hooks/useFacetCallback.ts index 92a3541a..938cb5cd 100644 --- a/packages/@react-facet/core/src/hooks/useFacetCallback.ts +++ b/packages/@react-facet/core/src/hooks/useFacetCallback.ts @@ -59,7 +59,6 @@ export function useFacetCallback[], T extends [...Y] // eslint-disable-next-line react-hooks/exhaustive-deps const callbackMemoized = useCallback(callback, dependencies) - // eslint-disable-next-line react-hooks/exhaustive-deps return useCallback( (...args: K) => { const values = facets.map((facet) => facet.get()) @@ -70,6 +69,8 @@ export function useFacetCallback[], T extends [...Y] return callbackMemoized(...(values as ExtractFacetValues))(...(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], ) }