New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor findNodeHandler #6736

Merged

piaskowyk merged 13 commits into main from @piaskowyk/findnodehandler-refactor

Dec 4, 2024

Member

piaskowyk commented Nov 20, 2024 •

edited

Loading

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


          Refactor findNodeHandler

93578ee

piaskowyk mentioned this pull request

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

Closed


          Update Fabric

a4204f0

tjzel approved these changes

View reviewed changes

Collaborator

tjzel left a comment

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 Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/createAnimatedComponent/createAnimatedComponent.tsx Show resolved Hide resolved

packages/react-native-reanimated/src/createAnimatedComponent/createAnimatedComponent.tsx Outdated Show resolved Hide resolved

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

View reviewed changes

packages/react-native-reanimated/src/createAnimatedComponent/NativeEventsManager.ts Show resolved Hide resolved

packages/react-native-reanimated/src/createAnimatedComponent/NativeEventsManager.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/createAnimatedComponent/NativeEventsManager.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/createAnimatedComponent/NativeEventsManager.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/createAnimatedComponent/NativeEventsManager.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/createAnimatedComponent/createAnimatedComponent.tsx Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/createAnimatedComponent/commonTypes.ts Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved

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

Replace findNodeHandle with reading _componentViewTag #6720

Merged


          Merge

tomekzaw reviewed

View reviewed changes

packages/react-native-reanimated/src/createAnimatedComponent/NativeEventsManager.ts Show resolved Hide resolved

packages/react-native-reanimated/src/createAnimatedComponent/createAnimatedComponent.tsx Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/fabricUtils.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved

tjzel reviewed

View reviewed changes

packages/react-native-reanimated/src/createAnimatedComponent/NativeEventsManager.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved


          Improvements

6a0c4c6

piaskowyk requested review from tjzel and tomekzaw

November 25, 2024 10:30

tomekzaw mentioned this pull request

Add support for React Native 0.77 #6768

Draft


          Add missing comment

7a41e4e

tjzel approved these changes

View reviewed changes

Collaborator

tjzel left a comment

Let's go!

packages/react-native-reanimated/src/platform-specific/findHostInstance.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/findHostInstance.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved


          Types improvements

b4e1063

tomekzaw approved these changes

View reviewed changes

piaskowyk enabled auto-merge

December 4, 2024 10:08

piaskowyk added this pull request to the merge queue

Merged via the queue into main with commit 5ffa477

9 checks passed

piaskowyk deleted the @piaskowyk/findnodehandler-refactor branch

December 4, 2024 10:09

tsapeta mentioned this pull request

Expo52: Can not animate BlurView intensity with reanimated expo/expo#32781

Open

rjantz2 mentioned this pull request

Animated.View produces errors when StrictMode is enabled #6754

Open

piaskowyk added a commit that referenced this pull request


          cherry-pick: Refactor findNodeHandler (#6736)

64cbdf7

piaskowyk added a commit that referenced this pull request


          cherry-pick: Refactor findNodeHandler (#6736)

80ba01f

piaskowyk mentioned this pull request

Release 3.16.5 #6804

Merged

piaskowyk added a commit that referenced this pull request


          cherry-pick: Refactor findNodeHandler (#6736)

574ebb5

tjzel pushed a commit that referenced this pull request


          Refactor findNodeHandler (#6736)

ca4f9f0

## Summary

This PR addresses the issue reported at
[https://github.com/software-mansion/react-native-reanimated/issues/6719](https://github.com/software-mansion/react-native-reanimated/issues/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:
[https://github.com/expo/expo/pull/33016](https://github.com/expo/expo/pull/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:
- #6030
- #5960
- #4445
- expo/expo#33016

## Test plan

- [x] check Paper
- [x] check Fabric
- [x] check Web

tjzel pushed a commit that referenced this pull request


          Refactor findNodeHandler (#6736)

d381387

## Summary

This PR addresses the issue reported at
[https://github.com/software-mansion/react-native-reanimated/issues/6719](https://github.com/software-mansion/react-native-reanimated/issues/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:
[https://github.com/expo/expo/pull/33016](https://github.com/expo/expo/pull/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:
- #6030
- #5960
- #4445
- expo/expo#33016

## Test plan

- [x] check Paper
- [x] check Fabric
- [x] check Web

tjzel pushed a commit that referenced this pull request


          Refactor findNodeHandler (#6736)

e64257a

## Summary

This PR addresses the issue reported at
[https://github.com/software-mansion/react-native-reanimated/issues/6719](https://github.com/software-mansion/react-native-reanimated/issues/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:
[https://github.com/expo/expo/pull/33016](https://github.com/expo/expo/pull/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:
- #6030
- #5960
- #4445
- expo/expo#33016

## Test plan

- [x] check Paper
- [x] check Fabric
- [x] check Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet