From da13cc10c9d3ddd1f4b6166cf20b0b4515ddcf3e Mon Sep 17 00:00:00 2001 From: Paulo Date: Thu, 25 Jul 2024 14:34:44 +0200 Subject: [PATCH 1/4] Fix test catching the bug --- .../core/src/hooks/useFacetUnwrap.spec.tsx | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/packages/@react-facet/core/src/hooks/useFacetUnwrap.spec.tsx b/packages/@react-facet/core/src/hooks/useFacetUnwrap.spec.tsx index ec734aa2..b2aea61c 100644 --- a/packages/@react-facet/core/src/hooks/useFacetUnwrap.spec.tsx +++ b/packages/@react-facet/core/src/hooks/useFacetUnwrap.spec.tsx @@ -219,7 +219,7 @@ it('does not trigger a re-render when changing a facet from undefined to undefin }) it('supports custom equality checks', () => { - const value = {} + const value = { prop: 'initial' } const demoFacet = createFacet({ initialValue: value }) // Dummy equality check that always returns its not equal @@ -258,4 +258,18 @@ it('supports custom equality checks', () => { expect(check).toHaveBeenCalledTimes(1) // but the check should be executed expect(check).toHaveBeenCalledWith(value) // passing the value (which should be the same) expect(renderedMock).toHaveBeenCalledTimes(1) // and since the equality check always returns "false", we have a render + + jest.clearAllMocks() + + const newValue = { prop: 'new' } + + // If we update with a new object, + act(() => { + demoFacet.set(newValue) + }) + + expect(equalityCheck).toHaveBeenCalledTimes(0) // equality check was already initialized + expect(check).toHaveBeenCalledTimes(1) // but the check should be executed + expect(check).toHaveBeenCalledWith(newValue) // passing the new value + expect(renderedMock).toHaveBeenCalledTimes(1) // and since the equality check always returns "false", we have a render }) From f1bc2dbeb373c3e8a032cb73d0a7197edd890636 Mon Sep 17 00:00:00 2001 From: Paulo Date: Thu, 25 Jul 2024 14:35:49 +0200 Subject: [PATCH 2/4] Properly checks if the new value is the same as the previous one The prior implementation would always compare the previous with itself --- packages/@react-facet/core/src/hooks/useFacetUnwrap.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@react-facet/core/src/hooks/useFacetUnwrap.ts b/packages/@react-facet/core/src/hooks/useFacetUnwrap.ts index c2bba377..1c2fe9a5 100644 --- a/packages/@react-facet/core/src/hooks/useFacetUnwrap.ts +++ b/packages/@react-facet/core/src/hooks/useFacetUnwrap.ts @@ -60,7 +60,7 @@ export function useFacetUnwrap( return { value } } - if (previousValue !== NO_VALUE && isEqual(previousValue)) { + if (previousValue !== NO_VALUE && isEqual(value)) { return previousState } From dd5397298bba6d47fc012ec0746d0d373991fb3a Mon Sep 17 00:00:00 2001 From: Paulo Date: Thu, 25 Jul 2024 14:37:02 +0200 Subject: [PATCH 3/4] Refactor so that in never calls setState, unless needed --- .../core/src/hooks/useFacetUnwrap.ts | 73 ++++++++++--------- 1 file changed, 38 insertions(+), 35 deletions(-) diff --git a/packages/@react-facet/core/src/hooks/useFacetUnwrap.ts b/packages/@react-facet/core/src/hooks/useFacetUnwrap.ts index 1c2fe9a5..2194a05d 100644 --- a/packages/@react-facet/core/src/hooks/useFacetUnwrap.ts +++ b/packages/@react-facet/core/src/hooks/useFacetUnwrap.ts @@ -1,4 +1,4 @@ -import { useLayoutEffect, useState } from 'react' +import { useLayoutEffect, useRef, useState } from 'react' import { FacetProp, isFacet, Value, NoValue, EqualityCheck, NO_VALUE } from '../types' import { defaultEqualityCheck } from '../equalityChecks' @@ -12,12 +12,15 @@ export function useFacetUnwrap( prop: FacetProp, equalityCheck: EqualityCheck = defaultEqualityCheck, ): T | NoValue { + const previousStateRef = useRef(NO_VALUE) + const [state, setState] = useState<{ value: T | NoValue }>(() => { if (!isFacet(prop)) return { value: prop } - return { - value: prop.get(), - } + const value = prop.get() + previousStateRef.current = value + + return { value } }) useLayoutEffect(() => { @@ -30,42 +33,42 @@ export function useFacetUnwrap( } return prop.observe((value) => { - setState((previousState) => { - const { value: previousValue } = previousState + const previousValue = previousStateRef.current + previousStateRef.current = value - /** - * Performs this equality check locally to prevent triggering two consecutive renderings - * for facets that have immutable values (unfortunately we can't handle mutable values). - * - * The two renderings might happen for the same state value if the Facet has a value on mount. - * - * The unwrap will get the value: - * - Once on initialization of the useState above - * - And another time on this observe initialization - */ - if (equalityCheck === defaultEqualityCheck) { - const typeofValue = typeof previousValue + /** + * Performs this equality check locally to prevent triggering two consecutive renderings + * for facets that have immutable values (unfortunately we can't handle mutable values). + * + * The two renderings might happen for the same state value if the Facet has a value on mount. + * + * The unwrap will get the value: + * - Once on initialization of the useState above + * - And another time on this observe initialization + */ + if (equalityCheck === defaultEqualityCheck) { + const typeofValue = typeof previousValue - if ( - (typeofValue === 'number' || - typeofValue === 'string' || - typeofValue === 'boolean' || - value === undefined || - value === null) && - value === previousValue - ) { - return previousState - } - - return { value } + if ( + (typeofValue === 'number' || + typeofValue === 'string' || + typeofValue === 'boolean' || + value === undefined || + value === null) && + value === previousValue + ) { + return } - if (previousValue !== NO_VALUE && isEqual(value)) { - return previousState - } + setState({ value }) + return + } + + if (previousValue !== NO_VALUE && isEqual(value)) { + return + } - return { value } - }) + setState({ value }) }) } }, [prop, equalityCheck]) From a3e37a3eebd4e02ff74e5a64593955deac7ea7ee Mon Sep 17 00:00:00 2001 From: Paulo Date: Thu, 25 Jul 2024 14:42:16 +0200 Subject: [PATCH 4/4] Revert "Refactor so that in never calls setState, unless needed" This reverts commit dd5397298bba6d47fc012ec0746d0d373991fb3a. --- .../core/src/hooks/useFacetUnwrap.ts | 73 +++++++++---------- 1 file changed, 35 insertions(+), 38 deletions(-) diff --git a/packages/@react-facet/core/src/hooks/useFacetUnwrap.ts b/packages/@react-facet/core/src/hooks/useFacetUnwrap.ts index 2194a05d..1c2fe9a5 100644 --- a/packages/@react-facet/core/src/hooks/useFacetUnwrap.ts +++ b/packages/@react-facet/core/src/hooks/useFacetUnwrap.ts @@ -1,4 +1,4 @@ -import { useLayoutEffect, useRef, useState } from 'react' +import { useLayoutEffect, useState } from 'react' import { FacetProp, isFacet, Value, NoValue, EqualityCheck, NO_VALUE } from '../types' import { defaultEqualityCheck } from '../equalityChecks' @@ -12,15 +12,12 @@ export function useFacetUnwrap( prop: FacetProp, equalityCheck: EqualityCheck = defaultEqualityCheck, ): T | NoValue { - const previousStateRef = useRef(NO_VALUE) - const [state, setState] = useState<{ value: T | NoValue }>(() => { if (!isFacet(prop)) return { value: prop } - const value = prop.get() - previousStateRef.current = value - - return { value } + return { + value: prop.get(), + } }) useLayoutEffect(() => { @@ -33,42 +30,42 @@ export function useFacetUnwrap( } return prop.observe((value) => { - const previousValue = previousStateRef.current - previousStateRef.current = value + setState((previousState) => { + const { value: previousValue } = previousState - /** - * Performs this equality check locally to prevent triggering two consecutive renderings - * for facets that have immutable values (unfortunately we can't handle mutable values). - * - * The two renderings might happen for the same state value if the Facet has a value on mount. - * - * The unwrap will get the value: - * - Once on initialization of the useState above - * - And another time on this observe initialization - */ - if (equalityCheck === defaultEqualityCheck) { - const typeofValue = typeof previousValue + /** + * Performs this equality check locally to prevent triggering two consecutive renderings + * for facets that have immutable values (unfortunately we can't handle mutable values). + * + * The two renderings might happen for the same state value if the Facet has a value on mount. + * + * The unwrap will get the value: + * - Once on initialization of the useState above + * - And another time on this observe initialization + */ + if (equalityCheck === defaultEqualityCheck) { + const typeofValue = typeof previousValue - if ( - (typeofValue === 'number' || - typeofValue === 'string' || - typeofValue === 'boolean' || - value === undefined || - value === null) && - value === previousValue - ) { - return - } + if ( + (typeofValue === 'number' || + typeofValue === 'string' || + typeofValue === 'boolean' || + value === undefined || + value === null) && + value === previousValue + ) { + return previousState + } - setState({ value }) - return - } + return { value } + } - if (previousValue !== NO_VALUE && isEqual(value)) { - return - } + if (previousValue !== NO_VALUE && isEqual(value)) { + return previousState + } - setState({ value }) + return { value } + }) }) } }, [prop, equalityCheck])