Skip to content
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.

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

Make useEvent return a different reference if event handler should be reattached #5743

Merged
merged 3 commits into from
Mar 6, 2024

Conversation

tjzel
Copy link
Collaborator

@tjzel tjzel commented Feb 28, 2024

Summary

Currently Reanimated relies on reattaching event handlers on render. However, we give any notice to React that it should occur, because useEvent always returns the same reference.

With these changes, the reference for a given handler will change if it had to be rebuild and components using memo will get re-rendered.

Fixes #5364

Test plan

See added MemoExample.tsx.

@tjzel tjzel added this pull request to the merge queue Mar 6, 2024
Merged via the queue into main with commit 89f76a7 Mar 6, 2024
7 checks passed
@tjzel tjzel deleted the @tjzel/memoization/useEvent branch March 6, 2024 10:26
github-merge-queue bot pushed a commit that referenced this pull request Mar 6, 2024
#5764)

## Summary

Fixing regression I introduced in #5743 by making the line

`const component = animatedRef.current` 

outside of `useEfect`.

## Test plan

Check `Article progress` example.
j-piasecki pushed a commit to software-mansion/react-native-gesture-handler that referenced this pull request Apr 29, 2024
## Description

This PR resolves the recognition issue of the Reanimated Event handler
for the previous Gesture Handler API. This adjustment was necessary due
to a modification in the internal property name that was utilized to
identify the Reanimated event handler (as seen here:
software-mansion/react-native-reanimated#5743).

## Test plan

Run this example -
https://github.com/software-mansion/react-native-reanimated/blob/main/app/src/examples/DragAndSnapExample.tsx
initRef.current.updateWorklet(handler);
const workletEventHandler = initRef.current.workletEventHandler;
workletEventHandler.updateWorklet(handler);
initRef.current = { workletEventHandler };
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way that reassignment during render can be avoided? This still violates React's rules — it means a render is "observable" even if it has not been committed. In React, this can happen with newer features like Transitions. If you update state in a Transition, but this Transition has not completed yet, it should not reflect in the user interface. However, assigning a mutable value during rendering and then reading from that value means that the change from an uncommitted Transition render would reflect for any further re-renders (even before this Transition commits — or even if it never ends up being committed).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaearon I think we could reassign with useEffect here, would that be good enough?

As a side note, I'm a bit confused with all these React features. From what you're saying it turns out that now you shouldn't assign to Refs during render - meaning that everything has to be pushed to useEffect. Is this intentional or am I missing some information here?

Copy link

@gaearon gaearon May 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not really new — in React, rendering is supposed to be pure. This means that the fact that some combination of props/state was rendered should be unobservable and shouldn't influence rendering output of other renders.

Do these look helpful? https://react.dev/reference/rules

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn, now I feel stupid 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stale closure with useAnimatedScrollHandler when passed to memo()
3 participants