Refactor findNodeHandler #6736

piaskowyk · 2024-11-20T14:59:55Z

Summary

This PR addresses the issue reported at #6719 and aims to:

Unify the method used to obtain the view tag, as there are currently several approaches.
Avoid passing a class component to the findNodeHandler. Instead, we'll pass a ref to the component, similar to what Expo implemented here: [core] Use findNodeHandle with a native ref instead of class component instance expo/expo#33016.
Limit unnecessary invocations of findNodeHandler to no more than one call per render.
Remove the invocation of findHostInstance from Paper renderer on the New Architecture.

Additional Remarks:

When a class component is passed to createAnimatedComponent, it will still fall back to the slow path where we can get a native ref.
In NativeEventManager, we need to call findNodeHandler again after every render to ensure that the children of <GestureDetector /> haven't changed their native tags.
LayoutAnimationConfig still uses findNodeHandler. It requires a complete refactor of their functionality to eliminate its use, and I plan to handle this in another PR.
findHostInstance_DEPRECATED always follows the slow path even for native refs, which is why I've implemented our own version of findHostInstance to optimize the happy path.

Fixes #6719

Related PRs:

Test plan

check Paper
check Fabric
check Web

tjzel

Looks good to me overall. Make sure to test it with the newest Gesture Handler and SVG on their more complex examples, since they used to have cases where these refs mechanisms broke.

packages/react-native-reanimated/src/createAnimatedComponent/createAnimatedComponent.tsx

packages/react-native-reanimated/src/createAnimatedComponent/NativeEventsManager.ts

packages/react-native-reanimated/src/createAnimatedComponent/createAnimatedComponent.tsx

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts

packages/react-native-reanimated/src/createAnimatedComponent/commonTypes.ts

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts

tomekzaw · 2024-11-22T22:04:46Z

packages/react-native-reanimated/src/createAnimatedComponent/NativeEventsManager.ts

-            ? this.#managedComponent
-            : componentAnimatedRef
-        ) ?? -1;
+      if (typeof scrollableNode === 'number') {


Does this also work as intended on Fabric where there are shadow node wrappers instead of view tags (numbers)?

On both platforms, getScrollableNode returns a number - I've checked it for both ScrollView and FlatList.

packages/react-native-reanimated/src/createAnimatedComponent/createAnimatedComponent.tsx

packages/react-native-reanimated/src/fabricUtils.ts

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts

tomekzaw · 2024-11-22T22:08:50Z

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts

+    // This is a native ref to a Paper component
+    return ref;
+  }
+  return undefined;


Shouldn't we throw an error in such case for easier debugging?

When findHostInstanceFastPath returns undefined we call findHostInstance_DEPRECATED()

This is not an error. That just means it’s a ref to a non-native component, and it’s necessary to call findHostInstance_DEPRECATED on them - I'll add a proper comment to code to explain it.

tomekzaw · 2024-11-22T22:10:28Z

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts

+    } catch (e) {
+      findHostInstance_DEPRECATED = (_ref: unknown) => null;
+    }


When will that happen? Shouldn't we throw an error indeed?

This is the previous behavior. I wonder if we should change it 🤔

I've just followed the previous implementation, but it might be more reasonable to throw an error instead

tomekzaw · 2024-11-22T22:11:29Z

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts

+
+  resolveFindHostInstance_DEPRECATED();
+  return findHostInstance_DEPRECATED(
+    isFabric() ? component : component._componentRef


Looks like we can skip this isFabric() call here and just call a function that uses findHostInstance_DEPRECATED under the hood.

What do you mean?

I wonder if we can call isFabric() only once to determine which implementation should be called but now I think there's a risk that isFabric() will be called too early.

packages/react-native-reanimated/src/createAnimatedComponent/NativeEventsManager.ts

tjzel · 2024-11-25T09:53:38Z

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts

+    // This is a native ref to a Paper component
+    return ref;
+  }
+  return undefined;


When findHostInstanceFastPath returns undefined we call findHostInstance_DEPRECATED()

tjzel · 2024-11-25T09:54:46Z

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts

+    } catch (e) {
+      findHostInstance_DEPRECATED = (_ref: unknown) => null;
+    }


This is the previous behavior. I wonder if we should change it 🤔

tjzel · 2024-11-25T09:55:11Z

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts

+
+  resolveFindHostInstance_DEPRECATED();
+  return findHostInstance_DEPRECATED(
+    isFabric() ? component : component._componentRef


What do you mean?

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts

Refactor findNodeHandler

93578ee

piaskowyk mentioned this pull request Nov 20, 2024

Renimated calls findNodeHandle with class instances, which is a slow path in React #6719

Open

Update Fabric

a4204f0

tjzel approved these changes Nov 21, 2024

View reviewed changes

piaskowyk added 4 commits November 22, 2024 09:37

Update events

37b6c87

Linter

835aca5

Improvements after review

43305eb

Conditions reorder

f699fb7

piaskowyk marked this pull request as ready for review November 22, 2024 09:07

piaskowyk requested review from tjzel and tomekzaw November 22, 2024 09:07

tjzel requested changes Nov 22, 2024

View reviewed changes

piaskowyk added 3 commits November 22, 2024 13:20

Improvements after review

fdeee9f

Improve Fabric-Paper compatibility

44fe723

Remove console log

c169616

piaskowyk requested a review from tjzel November 22, 2024 12:42

piaskowyk mentioned this pull request Nov 22, 2024

Replace findNodeHandle with reading _componentViewTag #6720

Merged

Merge

3808092

tomekzaw reviewed Nov 22, 2024

View reviewed changes

tjzel reviewed Nov 25, 2024

View reviewed changes

Improvements

6a0c4c6

piaskowyk requested review from tjzel and tomekzaw November 25, 2024 10:30

tomekzaw mentioned this pull request Nov 27, 2024

Add support for React Native 0.77 #6768

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor findNodeHandler #6736

Refactor findNodeHandler #6736

piaskowyk commented Nov 20, 2024 •

edited

Loading

tjzel left a comment

tomekzaw Nov 22, 2024

piaskowyk Nov 25, 2024

tomekzaw Nov 22, 2024

tjzel Nov 25, 2024

piaskowyk Nov 25, 2024

tomekzaw Nov 22, 2024

tjzel Nov 25, 2024

piaskowyk Nov 25, 2024

tomekzaw Nov 22, 2024

tjzel Nov 25, 2024

tomekzaw Nov 25, 2024

tjzel Nov 25, 2024

tjzel Nov 25, 2024

tjzel Nov 25, 2024

Refactor findNodeHandler #6736

Are you sure you want to change the base?

Refactor findNodeHandler #6736

Conversation

piaskowyk commented Nov 20, 2024 • edited Loading

Summary

Test plan

tjzel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piaskowyk commented Nov 20, 2024 •

edited

Loading