From 40c7736a1d12dffa5223598bfa3e8336d2bdc7a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Tue, 28 Nov 2023 16:36:35 -0800 Subject: [PATCH] Fix incorrect wrapping of refs in Image component (#41679) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/41679 This fixes a bug in the original implementation of image attached callbacks (still experimental). The problem was that we were unconditionally caching the ref passed to the underlying image component, which meant that whenever users passed new ref setters we wouldn't call them again. This fixes that by forcing the creation of a new ref value whenever a new ref is passed to the image component. Changelog: [internal] Reviewed By: jehartzog Differential Revision: D51618512 fbshipit-source-id: ac15160c528563c2131e8b3444dea4a6096f20bc --- .../Libraries/Image/Image.android.js | 13 +++- .../react-native/Libraries/Image/Image.ios.js | 12 +++- .../Libraries/Image/ImageInjection.js | 15 +---- .../Libraries/Image/__tests__/Image-test.js | 61 +++++++++++++++++++ 4 files changed, 85 insertions(+), 16 deletions(-) diff --git a/packages/react-native/Libraries/Image/Image.android.js b/packages/react-native/Libraries/Image/Image.android.js index e5c5d9f223b..eeec72e2724 100644 --- a/packages/react-native/Libraries/Image/Image.android.js +++ b/packages/react-native/Libraries/Image/Image.android.js @@ -15,10 +15,11 @@ import type {AbstractImageAndroid, ImageAndroid} from './ImageTypes.flow'; import flattenStyle from '../StyleSheet/flattenStyle'; import StyleSheet from '../StyleSheet/StyleSheet'; import TextAncestor from '../Text/TextAncestor'; +import useMergeRefs from '../Utilities/useMergeRefs'; import ImageAnalyticsTagContext from './ImageAnalyticsTagContext'; import { unstable_getImageComponentDecorator, - useWrapRefWithImageAttachedCallbacks, + useRefWithImageAttachedCallbacks, } from './ImageInjection'; import {getImageSourcesFromImageProps} from './ImageSourceUtils'; import {convertObjectFitToResizeMode} from './ImageUtils'; @@ -199,7 +200,15 @@ let BaseImage: AbstractImageAndroid = React.forwardRef( const resizeMode = objectFit || props.resizeMode || style?.resizeMode || 'cover'; - const actualRef = useWrapRefWithImageAttachedCallbacks(forwardedRef); + const imageAttachedCallbacksRef = useRefWithImageAttachedCallbacks(); + + const actualRef = + useMergeRefs | null>( + // $FlowFixMe[incompatible-call] + forwardedRef, + // $FlowFixMe[incompatible-call] + imageAttachedCallbacksRef, + ); return ( diff --git a/packages/react-native/Libraries/Image/Image.ios.js b/packages/react-native/Libraries/Image/Image.ios.js index ce21b5b4edf..2dabd798485 100644 --- a/packages/react-native/Libraries/Image/Image.ios.js +++ b/packages/react-native/Libraries/Image/Image.ios.js @@ -15,10 +15,11 @@ import type {AbstractImageIOS, ImageIOS} from './ImageTypes.flow'; import {createRootTag} from '../ReactNative/RootTag'; import flattenStyle from '../StyleSheet/flattenStyle'; import StyleSheet from '../StyleSheet/StyleSheet'; +import useMergeRefs from '../Utilities/useMergeRefs'; import ImageAnalyticsTagContext from './ImageAnalyticsTagContext'; import { unstable_getImageComponentDecorator, - useWrapRefWithImageAttachedCallbacks, + useRefWithImageAttachedCallbacks, } from './ImageInjection'; import {getImageSourcesFromImageProps} from './ImageSourceUtils'; import {convertObjectFitToResizeMode} from './ImageUtils'; @@ -161,7 +162,14 @@ let BaseImage: AbstractImageIOS = React.forwardRef((props, forwardedRef) => { }; const accessibilityLabel = props['aria-label'] ?? props.accessibilityLabel; - const actualRef = useWrapRefWithImageAttachedCallbacks(forwardedRef); + const imageAttachedCallbacksRef = useRefWithImageAttachedCallbacks(); + + const actualRef = useMergeRefs | null>( + // $FlowFixMe[incompatible-call] + forwardedRef, + // $FlowFixMe[incompatible-call] + imageAttachedCallbacksRef, + ); return ( diff --git a/packages/react-native/Libraries/Image/ImageInjection.js b/packages/react-native/Libraries/Image/ImageInjection.js index 38515d46e32..7a2e59ca9a4 100644 --- a/packages/react-native/Libraries/Image/ImageInjection.js +++ b/packages/react-native/Libraries/Image/ImageInjection.js @@ -52,19 +52,10 @@ export function unstable_unregisterImageAttachedCallback( imageAttachedCallbacks.delete(callback); } -type ProxyRef = (ImageInstance | null) => void; - -export function useWrapRefWithImageAttachedCallbacks( - forwardedRef?: React.Ref, -): ProxyRef { +export function useRefWithImageAttachedCallbacks(): React.RefSetter { const pendingCleanupCallbacks = useRef void>>([]); - const proxyRef = useRef(node => { - if (typeof forwardedRef === 'function') { - forwardedRef(node); - } else if (typeof forwardedRef === 'object' && forwardedRef != null) { - forwardedRef.current = node; - } + const ref = useRef((node: ImageInstance | null) => { if (node == null) { if (pendingCleanupCallbacks.current.length > 0) { pendingCleanupCallbacks.current.forEach(cb => cb()); @@ -80,5 +71,5 @@ export function useWrapRefWithImageAttachedCallbacks( } }); - return proxyRef.current; + return ref.current; } diff --git a/packages/react-native/Libraries/Image/__tests__/Image-test.js b/packages/react-native/Libraries/Image/__tests__/Image-test.js index 4cc8d11176d..46124adc88e 100644 --- a/packages/react-native/Libraries/Image/__tests__/Image-test.js +++ b/packages/react-native/Libraries/Image/__tests__/Image-test.js @@ -45,6 +45,67 @@ describe('', () => { expect(instance).toMatchSnapshot(); }); + it('should invoke original ref callbacks correctly when using image attached callbacks', () => { + jest.dontMock('../Image'); + + let imageInstanceFromCallback = null; + let imageInstanceFromRef1 = null; + let imageInstanceFromRef2 = null; + + const callback = jest.fn((instance: ElementRef) => { + imageInstanceFromCallback = instance; + + return () => { + imageInstanceFromCallback = null; + }; + }); + + ImageInjection.unstable_registerImageAttachedCallback(callback); + + expect(imageInstanceFromCallback).toBe(null); + + let testRenderer; + + const ref1 = jest.fn(instance => { + imageInstanceFromRef1 = instance; + }); + + act(() => { + testRenderer = create(); + }); + + expect(imageInstanceFromCallback).not.toBe(null); + expect(imageInstanceFromRef1).not.toBe(null); + expect(imageInstanceFromCallback).toBe(imageInstanceFromRef1); + expect(callback).toHaveBeenCalledTimes(1); + expect(ref1).toHaveBeenCalledTimes(1); + + const ref2 = jest.fn( + (instance: React.ElementRef | null): void => { + imageInstanceFromRef2 = instance; + }, + ); + + act(() => { + testRenderer.update(); + }); + + expect(imageInstanceFromCallback).not.toBe(null); + expect(imageInstanceFromRef1).toBe(null); + expect(imageInstanceFromRef2).not.toBe(null); + expect(imageInstanceFromCallback).toBe(imageInstanceFromRef2); + expect(callback).toHaveBeenCalledTimes(2); + expect(ref1).toHaveBeenCalledTimes(2); + expect(ref2).toHaveBeenCalledTimes(1); + + act(() => { + testRenderer.update(); + }); + + expect(callback).toHaveBeenCalledTimes(2); + expect(ref2).toHaveBeenCalledTimes(1); + }); + it('should call image attached callbacks (basic)', () => { jest.dontMock('../Image');