diff --git a/packages/react-native/Libraries/Image/ImageInjection.js b/packages/react-native/Libraries/Image/ImageInjection.js index 60feb3999238..088b85b1251e 100644 --- a/packages/react-native/Libraries/Image/ImageInjection.js +++ b/packages/react-native/Libraries/Image/ImageInjection.js @@ -13,7 +13,7 @@ import type {AbstractImageAndroid, AbstractImageIOS} from './ImageTypes.flow'; import useMergeRefs from '../Utilities/useMergeRefs'; import * as React from 'react'; -import {useRef} from 'react'; +import {useCallback} from 'react'; type ImageComponentDecorator = (AbstractImageAndroid => AbstractImageAndroid) & (AbstractImageIOS => AbstractImageIOS); @@ -53,32 +53,21 @@ export function unstable_unregisterImageAttachedCallback( export function useWrapRefWithImageAttachedCallbacks( forwardedRef: React.RefSetter, ): React.RefSetter { - const pendingCleanupCallbacks = useRef void>>([]); - - const imageAttachedCallbacksRef = - useRef void>(null); - - if (imageAttachedCallbacksRef.current == null) { - imageAttachedCallbacksRef.current = (node: ImageInstance | null): void => { - if (node == null) { - if (pendingCleanupCallbacks.current.length > 0) { - pendingCleanupCallbacks.current.forEach(cb => cb()); - pendingCleanupCallbacks.current = []; - } - } else { - imageAttachedCallbacks.forEach(imageAttachedCallback => { - const maybeCleanupCallback = imageAttachedCallback(node); - if (maybeCleanupCallback != null) { - pendingCleanupCallbacks.current.push(maybeCleanupCallback); - } - }); + const attachCallback = useCallback((node: ImageInstance) => { + const pendingCleanup = []; + imageAttachedCallbacks.forEach(imageAttachedCallback => { + const maybeCleanupCallback = imageAttachedCallback(node); + if (maybeCleanupCallback != null) { + pendingCleanup.push(maybeCleanupCallback); } - }; - } + }); + return () => pendingCleanup.forEach(cb => cb()); + }, []); // `useMergeRefs` returns a stable ref if its arguments don't change. return useMergeRefs( forwardedRef, - imageAttachedCallbacksRef.current, + // $FlowFixMe[incompatible-type] - blocked on refined refsetter types + attachCallback as React.RefSetter, ); } diff --git a/packages/react-native/Libraries/Utilities/__tests__/useRefEffect-itest.js b/packages/react-native/Libraries/Utilities/__tests__/refCallbackCleanup-itest.js similarity index 69% rename from packages/react-native/Libraries/Utilities/__tests__/useRefEffect-itest.js rename to packages/react-native/Libraries/Utilities/__tests__/refCallbackCleanup-itest.js index d1fd9f0398d3..d4c69ba3b9ae 100644 --- a/packages/react-native/Libraries/Utilities/__tests__/useRefEffect-itest.js +++ b/packages/react-native/Libraries/Utilities/__tests__/refCallbackCleanup-itest.js @@ -13,24 +13,23 @@ import '@react-native/fantom/src/setUpDefaultReactNativeEnvironment'; import type {HostInstance} from '../../../src/private/types/HostInstance'; import View from '../../Components/View/View'; -import useRefEffect from '../useRefEffect'; +import useMergeRefs from '../useMergeRefs'; import * as Fantom from '@react-native/fantom'; import * as React from 'react'; type RegistryEntry = {kind: 'effect' | 'cleanup', name: string, key: ?string}; /** - * TestView provide a component execution environment to test hooks. + * TestView provide a component execution environment to test ref lifecycle. */ function TestView({ childKey = null, effect, }: { childKey: ?string, - effect: (?HostInstance) => (() => void) | void, + effect: React.RefSetter, }) { - const ref = useRefEffect(effect); - return ; + return ; } function keyOf(instance: ?HostInstance): ?string { @@ -49,8 +48,9 @@ function cleanupEntry(name: string, key: ?string): RegistryEntry { } function mockEffectRegistry(): { - mockEffect: string => (?HostInstance) => () => void, - mockEffectWithoutCleanup: string => (?HostInstance) => void, + mockEffect: string => React.RefSetter, + mockEffectThatThrows: string => React.RefSetter, + mockEffectWithoutCleanup: string => React.RefSetter, registry: Array, } { const registry: Array = []; @@ -64,6 +64,15 @@ function mockEffectRegistry(): { }; }; }, + mockEffectThatThrows(name: string): (?HostInstance) => void { + return instance => { + const key = keyOf(instance); + registry.push(effectEntry(name, key)); + if (instance != null) { + throw new Error(`${name} failed`); + } + }; + }, mockEffectWithoutCleanup(name: string): (?HostInstance) => void { return instance => { const key = keyOf(instance); @@ -90,7 +99,22 @@ test('calls effect without cleanup', () => { root.render(<>); }); - expect(registry).toEqual([effectEntry('A', 'foo')]); + expect(registry).toEqual([effectEntry('A', 'foo'), effectEntry('A', null)]); +}); + +test('calls effect with null when it throws', () => { + const root = Fantom.createRoot(); + + const {mockEffectThatThrows, registry} = mockEffectRegistry(); + const effectA = mockEffectThatThrows('A'); + + Fantom.runTask(() => { + root.render(); + }); + + // A cleanup is only adopted from an effect that returns normally, so React + // detaches by invoking the effect again with null. + expect(registry).toEqual([effectEntry('A', 'foo'), effectEntry('A', null)]); }); test('calls effect and cleanup', () => { @@ -177,6 +201,36 @@ test('calls cleanup and effect on new instance', () => { ]); }); +test('useMergeRefs correctly combines different ref handler types', () => { + const root = Fantom.createRoot(); + + const {mockEffect, mockEffectWithoutCleanup, registry} = mockEffectRegistry(); + const effectA = mockEffect('A'); + const effectB = mockEffectWithoutCleanup('B'); + + function ComponentUsingMergeRefs() { + const mergedRef = useMergeRefs(effectA, effectB); + return ; + } + + Fantom.runTask(() => { + root.render(); + }); + + expect(registry).toEqual([effectEntry('A', 'foo'), effectEntry('B', 'foo')]); + + Fantom.runTask(() => { + root.render(<>); + }); + + expect(registry).toEqual([ + effectEntry('A', 'foo'), + effectEntry('B', 'foo'), + cleanupEntry('A', 'foo'), + effectEntry('B', null), + ]); +}); + test('cleans up old effect before calling new effect with new instance', () => { const root = Fantom.createRoot(); diff --git a/packages/react-native/Libraries/Utilities/useMergeRefs.js b/packages/react-native/Libraries/Utilities/useMergeRefs.js index 3f76a0b3392d..31751e35637a 100644 --- a/packages/react-native/Libraries/Utilities/useMergeRefs.js +++ b/packages/react-native/Libraries/Utilities/useMergeRefs.js @@ -8,7 +8,6 @@ * @format */ -import useRefEffect from './useRefEffect'; import * as React from 'react'; import {useCallback} from 'react'; @@ -24,36 +23,28 @@ import {useCallback} from 'react'; export default function useMergeRefs( ...refs: ReadonlyArray> ): React.RefSetter { - const refEffect = useCallback( + // $FlowFixMe[incompatible-type] - blocked on refined refsetter types + return useCallback( (current: Instance) => { const cleanups: ReadonlyArray void)> = refs.map(ref => { - if (ref == null) { - return undefined; - } else { - if (typeof ref === 'function') { - // $FlowFixMe[incompatible-type] - Flow does not understand ref cleanup. - const cleanup: void | (() => void) = ref(current); - return typeof cleanup === 'function' - ? cleanup - : () => { - ref(null); - }; - } else { - ref.current = current; - return () => { - ref.current = null; - }; - } + if (typeof ref === 'function') { + // $FlowFixMe[incompatible-type] - Flow does not understand ref cleanup. + const cleanup: void | (() => void) = ref(current); + return typeof cleanup === 'function' + ? cleanup + : () => { + ref(null); + }; + } else if (ref != null) { + ref.current = current; + return () => { + ref.current = null; + }; } }); - return () => { - for (const cleanup of cleanups) { - cleanup?.(); - } - }; + return () => cleanups.forEach(cleanup => cleanup?.()); }, [...refs], // eslint-disable-line react-hooks/exhaustive-deps - ); - return useRefEffect(refEffect); + ) as React.RefSetter; } diff --git a/packages/react-native/Libraries/Utilities/useRefEffect.js b/packages/react-native/Libraries/Utilities/useRefEffect.js deleted file mode 100644 index e1e38a0dd7ac..000000000000 --- a/packages/react-native/Libraries/Utilities/useRefEffect.js +++ /dev/null @@ -1,43 +0,0 @@ -/** - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - * - * @flow strict - * @format - */ - -import {useCallback, useRef} from 'react'; - -/** - * Constructs a callback ref that provides similar semantics as `useEffect`. The - * supplied `effect` callback will be called with non-null component instances. - * The `effect` callback can also optionally return a cleanup function. - * - * When a component is updated or unmounted, the cleanup function is called. The - * `effect` callback will then be called again, if applicable. - * - * When a new `effect` callback is supplied, the previously returned cleanup - * function will be called before the new `effect` callback is called with the - * same instance. - * - * WARNING: The `effect` callback should be stable (e.g. using `useCallback`). - */ -export default function useRefEffect( - effect: TInstance => (() => void) | void, -): React.RefCallback { - const cleanupRef = useRef<(() => void) | void>(undefined); - return useCallback( - (instance: null | TInstance) => { - if (cleanupRef.current) { - cleanupRef.current(); - cleanupRef.current = undefined; - } - if (instance != null) { - cleanupRef.current = effect(instance); - } - }, - [effect], - ); -} diff --git a/packages/react-native/src/private/animated/createAnimatedPropsHook.js b/packages/react-native/src/private/animated/createAnimatedPropsHook.js index b6dcef3bd329..023592163c1f 100644 --- a/packages/react-native/src/private/animated/createAnimatedPropsHook.js +++ b/packages/react-native/src/private/animated/createAnimatedPropsHook.js @@ -16,7 +16,6 @@ import AnimatedProps from '../../../Libraries/Animated/nodes/AnimatedProps'; import AnimatedValue from '../../../Libraries/Animated/nodes/AnimatedValue'; import {isPublicInstance as isFabricPublicInstance} from '../../../Libraries/ReactNative/ReactFabricPublicInstance/ReactFabricPublicInstanceUtils'; import {RootTagContext} from '../../../Libraries/ReactNative/RootTag'; -import useRefEffect from '../../../Libraries/Utilities/useRefEffect'; import warnOnce from '../../../Libraries/Utilities/warnOnce'; import * as ReactNativeFeatureFlags from '../featureflags/ReactNativeFeatureFlags'; import {createAnimatedPropsMemoHook} from './createAnimatedPropsMemoHook'; @@ -112,7 +111,14 @@ export default function createAnimatedPropsHook( // But there is no way to transparently compose three separate callback refs, // so we just combine them all into one for now. const refEffect = useCallback( - (instance: TInstance) => { + (instance: TInstance | null) => { + // React only adopts the returned cleanup if this callback returns + // normally, so it falls back to re-invoking it with null to detach if a + // previous call threw. + if (instance == null) { + return; + } + // NOTE: This may be called more often than necessary (e.g. when `props` // changes), but `setNativeView` already optimizes for that. // $FlowFixMe[incompatible-type] @@ -224,9 +230,8 @@ export default function createAnimatedPropsHook( }, [node], ); - const callbackRef = useRefEffect(refEffect); - return [reduceAnimatedProps(node, props), callbackRef]; + return [reduceAnimatedProps(node, props), refEffect]; }; }