From d14120cf2c8d30034ee2715a8b54fd012aac3f46 Mon Sep 17 00:00:00 2001 From: Chris Miles Date: Wed, 5 Feb 2025 16:25:09 -0500 Subject: [PATCH 1/2] Fix duplicate child addition in AnimatedWithChildren and add test file --- .../__tests__/AnimatedWithChildren-test.js | 74 +++++++++++++++++++ .../Animated/nodes/AnimatedWithChildren.js | 5 +- 2 files changed, 78 insertions(+), 1 deletion(-) create mode 100644 packages/react-native/Libraries/Animated/__tests__/AnimatedWithChildren-test.js diff --git a/packages/react-native/Libraries/Animated/__tests__/AnimatedWithChildren-test.js b/packages/react-native/Libraries/Animated/__tests__/AnimatedWithChildren-test.js new file mode 100644 index 000000000000..bd82da1bdf1c --- /dev/null +++ b/packages/react-native/Libraries/Animated/__tests__/AnimatedWithChildren-test.js @@ -0,0 +1,74 @@ +/** + * 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. + * + * @format + * @oncall react_native + */ + +describe('AnimatedWithChildren', () => { + let AnimatedWithChildren; + let AnimatedValue; + + beforeEach(() => { + jest.resetModules(); + AnimatedWithChildren = require('../nodes/AnimatedWithChildren').default; + AnimatedValue = require('../nodes/AnimatedValue').default; + }); + + it('adds a child to the children array when it is not already present', () => { + const parent = new AnimatedWithChildren(); + const child = new AnimatedValue(0); + + expect(parent.__getChildren().length).toBe(0); + + parent.__addChild(child); + expect(parent.__getChildren().length).toBe(1); + expect(parent.__getChildren()).toContain(child); + }); + + it('prevents adding the same child more than once to the children array', () => { + const parent = new AnimatedWithChildren(); + const child = new AnimatedValue(0); + + parent.__addChild(child); + expect(parent.__getChildren().length).toBe(1); + + parent.__addChild(child); + expect(parent.__getChildren().length).toBe(1); + }); + + it('removes a child correctly from the children array when it exists', () => { + const parent = new AnimatedWithChildren(); + const child1 = new AnimatedValue(0); + const child2 = new AnimatedValue(10); + + parent.__addChild(child1); + parent.__addChild(child2); + expect(parent.__getChildren().length).toBe(2); + + parent.__removeChild(child1); + expect(parent.__getChildren().length).toBe(1); + expect(parent.__getChildren()).not.toContain(child1); + expect(parent.__getChildren()).toContain(child2); + + parent.__removeChild(child2); + expect(parent.__getChildren().length).toBe(0); + }); + + it('safely ignores removal calls for non-existent children in the children array', () => { + const parent = new AnimatedWithChildren(); + const child = new AnimatedValue(0); + + parent.__addChild(child); + expect(parent.__getChildren().length).toBe(1); + + parent.__removeChild(child); + expect(parent.__getChildren().length).toBe(0); + + expect(() => parent.__removeChild(child)).not.toThrow(); + expect(parent.__getChildren().length).toBe(0); + }); +}); diff --git a/packages/react-native/Libraries/Animated/nodes/AnimatedWithChildren.js b/packages/react-native/Libraries/Animated/nodes/AnimatedWithChildren.js index 9d4469e33c44..4cfa0b950e53 100644 --- a/packages/react-native/Libraries/Animated/nodes/AnimatedWithChildren.js +++ b/packages/react-native/Libraries/Animated/nodes/AnimatedWithChildren.js @@ -39,6 +39,10 @@ export default class AnimatedWithChildren extends AnimatedNode { } __addChild(child: AnimatedNode): void { + // Prevent adding duplicate animated nodes. + if (this._children.includes(child)) { + return; + } if (this._children.length === 0) { this.__attach(); } @@ -53,7 +57,6 @@ export default class AnimatedWithChildren extends AnimatedNode { __removeChild(child: AnimatedNode): void { const index = this._children.indexOf(child); if (index === -1) { - console.warn("Trying to remove a child that doesn't exist"); return; } if (this.__isNative && child.__isNative) { From 5afcda0104e4e3e5f0ae6f11776f4d8ec209aeda Mon Sep 17 00:00:00 2001 From: Chris Miles Date: Mon, 5 May 2025 11:40:45 -0400 Subject: [PATCH 2/2] Add test verifying 1:1 addChild/removeChild relationship without direct calls --- .../__tests__/AnimatedWithChildren-test.js | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/packages/react-native/Libraries/Animated/__tests__/AnimatedWithChildren-test.js b/packages/react-native/Libraries/Animated/__tests__/AnimatedWithChildren-test.js index bd82da1bdf1c..7630c7497480 100644 --- a/packages/react-native/Libraries/Animated/__tests__/AnimatedWithChildren-test.js +++ b/packages/react-native/Libraries/Animated/__tests__/AnimatedWithChildren-test.js @@ -11,11 +11,13 @@ describe('AnimatedWithChildren', () => { let AnimatedWithChildren; let AnimatedValue; + let AnimatedTransform; beforeEach(() => { jest.resetModules(); AnimatedWithChildren = require('../nodes/AnimatedWithChildren').default; AnimatedValue = require('../nodes/AnimatedValue').default; + AnimatedTransform = require('../nodes/AnimatedTransform').default; }); it('adds a child to the children array when it is not already present', () => { @@ -58,17 +60,24 @@ describe('AnimatedWithChildren', () => { expect(parent.__getChildren().length).toBe(0); }); - it('safely ignores removal calls for non-existent children in the children array', () => { - const parent = new AnimatedWithChildren(); - const child = new AnimatedValue(0); + it('maintains 1:1 relationship between addChild and removeChild when the same value is reused', () => { + const value = new AnimatedValue(0); + const initialChildCount = value.__getChildren().length; + expect(initialChildCount).toBe(0); - parent.__addChild(child); - expect(parent.__getChildren().length).toBe(1); + const transform = [ + { translateX: value }, + { translateY: value }, + ]; - parent.__removeChild(child); - expect(parent.__getChildren().length).toBe(0); + const animatedTransform = AnimatedTransform.from(transform); + animatedTransform.__attach(); - expect(() => parent.__removeChild(child)).not.toThrow(); - expect(parent.__getChildren().length).toBe(0); + const childCountAfterAttach = value.__getChildren().length; + expect(childCountAfterAttach).toBeGreaterThan(initialChildCount); + + animatedTransform.__detach(); + + expect(value.__getChildren().length).toBe(initialChildCount); }); });