-
-
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
Remove makeRemote
#5518
Merged
Merged
Remove makeRemote
#5518
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tomekzaw
requested changes
Dec 23, 2023
piaskowyk
reviewed
Jan 9, 2024
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.
Overall, everything is going great. However, I do have one doubt regarding potential regression.
tomekzaw
approved these changes
Jan 12, 2024
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.
🚢 🇮🇹
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 16, 2024
## Summary This PR fixes a regression introduced in #5518. `useHandler` works by maintaining a `context` object that can be accessed and modified in event handlers (e.g. within `useAnimatedGestureHandler`). Removing `makeRemote` call there made it so that any changes to the `context` object were not visible in event handlers, rendering the `context` object useless. In this PR I wrapped the context object with `makeShareable`, which works basically in the same way as `makeRemote` did. ## Test plan Check whether the `[SET] Modals` example behaves properly (i.e. if the modal closing gestures are captured correctly).
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 8, 2024
## Summary When we merged #5518 we weren't aware that the mechanism of `ShareableHandles` (objects with `__init` worklet property) are memoized on UI thread with the use of `std::unique_ptr` and a map. Because of that `remoteState` currently resets to its initial state after `dependencies` change. This happens, because: - worklet is stateful during one call of `runOnUI`, - `startMapper` calls `runOnUI` exactly once, - worklet has access to its state and can change it (and access changed state) as long as the mapper runs, - when `dependencies` change, mapper is stopped and started again, so another `runOnUI` is called and previous state is lost - worklet starts with initial instead ## Test plan <details> <summary> Test code </summary> ```tsx import React from 'react'; import { StyleSheet, View, Button } from 'react-native'; import Animated, { useAnimatedStyle, useSharedValue, withTiming, } from 'react-native-reanimated'; export default function EmptyExample() { const [render, setRender] = React.useState(0); const [deps, setDeps] = React.useState(0); const sv = useSharedValue(100); const animatedStyle = useAnimatedStyle(() => ({ width: sv.value }), [deps]); function handleRerender() { console.log('Rerendering'); setRender((prev) => prev + 1); } function handleChangeDeps() { console.log('Changing deps & Re-rendering'); setDeps((prev) => prev + 1); } function handleAnimate() { sv.value = withTiming(Math.random() * 100 + 50, { duration: 1000 }); } return ( <View style={styles.container}> <Button title="Rerender" onPress={handleRerender} /> <Button title="Animate" onPress={handleAnimate} /> <Button title="Change deps" onPress={handleChangeDeps} /> <Animated.View style={[styles.box, animatedStyle]} /> </View> ); } const styles = StyleSheet.create({ container: { flex: 1, alignItems: 'center', justifyContent: 'center', }, box: { height: 100, backgroundColor: 'red', }, }); ``` </details> Add this to `useAnimatedStyle`: ```diff diff --git a/src/reanimated2/hook/useAnimatedStyle.ts b/src/reanimated2/hook/useAnimatedStyle.ts index d6d015572..18dd46d17 100644 --- a/src/reanimated2/hook/useAnimatedStyle.ts +++ b/src/reanimated2/hook/useAnimatedStyle.ts @@ -505,6 +505,7 @@ For more, see the docs: \`https://docs.swmansion.com/react-native-reanimated/doc } else { fun = () => { 'worklet'; + console.log(remoteState.last); styleUpdater( shareableViewDescriptors, updaterFn, ``` |Before|After| |--|--| |![Screenshot 2024-02-07 at 18 20 03](https://github.com/software-mansion/react-native-reanimated/assets/40713406/47e0c24f-e1f2-49d6-8afb-f3c45cffa0e8)|![Screenshot 2024-02-07 at 18 21 15](https://github.com/software-mansion/react-native-reanimated/assets/40713406/e9f27133-3923-4c3f-a4a9-a80eecc535a0)|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
makeRemote
function seems to be no longer necessary.Prettifying
useAnimatedStyle
as a bonus.Why can
makeRemote
be removedmakeRemote
utilizes theShareableHandle
idea. Let's look at its code:The object
handle
defined inside this function has__init
worklet property. This worklet has the objectinitial
in its closure. WhenmakeShareableCloneRecursive
is called with this worklet,initial's
value will be copied to thehandle
's closure. Therefore, calling__init
on UI runtime will return the copied value of initial.So at this point we have a
ShareableObject
handle
with a worklet property__init
. Both this object and its worklet were saved in_shareableCache
whenmakeShareableCloneRecursive
was called. Current mappings are:_shareableCache[handle]
-handle
_shareableCache[handle.__init]
-handle.__init
Now
registerShareableMapping
is called. This function sets a new mapping for_shareableCache
. It mapsinitial
tohandle
. Therefore any worklet that hasinitial
in its closure will take the mapping forinitial
instead, which is thehandle
.Then on the UI thread, when we evaluate worklet that uses
initial
, we run it throughvalueUnpacker
.valueUnpacker
checks if the object has__init
property, if yes, then it return the result of__init
worklet. So it returns the copied value ofinitial
that was copied during evaluation of__init
.To sum it up:
makeRemote
is a function that maps a given JS reference to a function that on UI thread will return the same value. With current implementation of Shareables this hardly serves any purpose, since we could just copy the value like a plain JS value, using its copy.For example, code:
Provides the same results as
Since all that
makeRemote
does is add unnecessary identity unpacking function.Test plan
🚀
To check
areAnimationsActive
: