-
-
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
Better error when passing easing from 'react-native' instead of 'reanimated' #5639
Conversation
Co-authored-by: Tomek Zawadzki <tomasz.zawadzki@swmansion.com>
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.
Shouldn't we also add a check if this is a development build? We shouldn't be checking this in production I think.
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.
Please also add some test plan in the PR description (with before/after screenshots maybe?).
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.
Just make the comment more accurate and we are good to go!
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.
Looks good, left some questions in the comments.
Also, shouldn't we also perform the same check here:
- ComplexAnimationBuilder
Line 42 in deb741b
easing(easingFunction: EasingFunction): this { - CurvedTransition
react-native-reanimated/src/reanimated2/layoutReanimation/defaultTransitions/CurvedTransition.ts
Line 48 in deb741b
easingY(easing: EasingFunction): CurvedTransition { - JumpingTransition
react-native-reanimated/src/reanimated2/layoutReanimation/defaultTransitions/JumpingTransition.ts
Line 7 in deb741b
import { Easing } from '../../Easing'; - Keyframe
react-native-reanimated/src/reanimated2/layoutReanimation/animationBuilder/Keyframe.ts
Line 125 in deb741b
easing?: EasingFunction;
If yes, it probably makes sense to move it to a utility function assertEasingIsWorklet
and call it from all these places.
Co-authored-by: Tomasz Żelawski <40713406+tjzel@users.noreply.github.com>
@Latropos Can you check if it works on Web? |
@tjzel I've tested, it works on web |
Co-authored-by: Tomek Zawadzki <tomasz.zawadzki@swmansion.com>
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.
Looks good to me, let's just slightly improve the error message.
Co-authored-by: Tomek Zawadzki <tomasz.zawadzki@swmansion.com>
Co-authored-by: Krzysztof Piaskowy <krzysztof.piaskowy@swmansion.com>
Summary
Function
withTiming
should throw an error when easing is not a worklet (or a bound function run from UI thread).This is a common bug, because it happens when user is mixing imports from reanimated and animated.
Test plan
timing animation (example from the table) - code
Keyframe animation - code
Curved transition animation - code