-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Replace useRef with useState for useSharedValue #5458
Replace useRef with useState for useSharedValue #5458
Conversation
3a642cd
to
46a8b54
Compare
@amadeus Thanks a lot for submitting this PR! |
The general idea here is that we can avoid the makeMutable call on every single render. This change is also compatible with hot reloading.
46a8b54
to
5304613
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested it and everything works as expected 🎉 Thanks for the PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for the PR!
Yay Thanks! |
useEffect(() => { | ||
return () => { | ||
cancelAnimation(ref.current); | ||
cancelAnimation(mutable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of a drive-by comment but in general effects that only do cleanup usually indicate that something else is wrong. Effects should be symmetrical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d be curious for your thoughts on how something like this should work? Assuming you have an animation in progress that needs to be cleaned up on unmount
## Summary The general idea here is that we can avoid the `makeMutable` call on every single render. This change is also compatible with hot reloading. We (at Discord) have been running this patch on top of our own reanimated now for a few weeks and haven't noticed any side effects of the change. There has been a bit of additional discussion I've seen surrounding this issue here: #3199 ## Test plan N/A
Summary
The general idea here is that we can avoid the
makeMutable
call on every single render. This change is also compatible with hot reloading.We (at Discord) have been running this patch on top of our own reanimated now for a few weeks and haven't noticed any side effects of the change.
There has been a bit of additional discussion I've seen surrounding this issue here: #3199
Test plan
N/A