-
-
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
Add inline styles without animation functions #4062
Conversation
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.
This is much better 👏 👏
I left a couple of comments inline. I also think that we can relatively easily extend this now to also work for other non-style properties. We now use useAnimatedProps
but since it is just an alias for useAnimatedStyle
, making this inline styles approach work for non-style props should be very much limited to recognizing if shared value is used fo a given prop and then including it in newInlineStyles
object (ofc we would rename this variable in such case)
); | ||
|
||
if (hasChanged) { | ||
const sharableViewDescriptors = |
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.
Can we maybe initialized veiw descriptors lazily here? We only put at most one item there for inline styles. Alternatively we can change updateProps method to conditionally take viewTag instead of view descriptors structure that just seem wasteful here
|
||
const maybeViewRef = NativeReanimatedModule.native | ||
? undefined | ||
: ({ items: new Set([this]) } as ViewRefSet<any>); // see makeViewsRefSet |
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.
we can follow similar approach with view descriptors and only define them here, no?
src/createAnimatedComponent.tsx
Outdated
const newStyle: StyleProps = {}; | ||
for (const [key, styleValue] of Object.entries(style)) { | ||
if ( | ||
styleValue.value === undefined && |
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.
can we define a method isSharedValue
and use it here? I was thinking about adding such method for some time and it'll be useful in other places as well.
!(key === 'transform' && isInlineStyleTransform(styleValue)) | ||
) { | ||
newStyle[key] = styleValue; | ||
} |
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.
if it is shared value don't we still include it in newStyle
? Much like we do with useAnimatedStyle
where we just take the initial value, so here maybe we should do newStyle[key] = styleValue.value
src/createAnimatedComponent.tsx
Outdated
update[key] = styleValue.map((t: Record<string, any>) => | ||
Object.keys(t).reduce( | ||
(acc, curr) => ({ ...acc, [curr]: t[curr].value }), | ||
{} |
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 prefer to rewrite this bit using simple loops. One aspect is that object spread isn't typically very fast and another one is readability. Using map
method is ok, but reduce
with spread is a bit too much I believe
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 a few more comments inline
src/reanimated2/utils.ts
Outdated
|
||
export function isSharedValue(obj: any) { | ||
'worklet'; | ||
return typeof obj === 'object' && obj.value !== undefined; |
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.
would be better to add a new field that we can use to distinguish shared value w/o risking conflicts. I think with SVG we already have similar problems in the past as certain objects have value fields there. This new field can be added in mutables.ts
(ideally we should use JS Symbol instead of field with unique name but symbols are not yet supported to be sent to the ui thread).
In addition, this method should definition should look as follows:
export function isSharedValue<T>(value: any): value is SharedValue<T>
This way typescript would know that if the method return true, then the object is of certain type and we could avoid casting in the future.
plugin.js
Outdated
t.callExpression( | ||
t.memberExpression(t.identifier('console'), t.identifier('warn')), | ||
[ | ||
t.stringLiteral( |
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.
Would it make sense if instead of doing console.log(message)
here we'd generate require('react-native-reanimated).showUseOfValueInStyleWarning()
instead. This way, we could put the message in the library's code and hence we won't need to update snapshots every time we want to change it
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.
if (Object.keys(newInlineStyles).length) { | ||
this._inlineStylesMapperId = startMapper( | ||
updaterFunction, | ||
Object.values(newInlineStyles) |
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.
it seem like if there is a transform object with only one of transform elements being a shared value, this won't actually extract that shared value. Can you verify if this works ok?
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.
Surprisingly it works ok. See extractInputs
method in mappers.ts
. It extracts all shared values from an object and it's run on all inputs when starting the mapper.
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 for making all the updates.Looks good!
} | ||
|
||
function processInlineStylesWarning(t, path, state) { | ||
if (state.opts.disableInlineStylesWarning) return; |
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.
We should only generate warning for DEV builds. In prod you won't see the warning appear anyways.
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.
There is isRelease
function used in the plugin that you can call here to check the build variation
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please follow the template so that the reviewers can easily understand what the code changes affect. --> ## Summary Based on #4062 Adds support for inline animated props like this: ```js const AnimatedCircle = Animated.createAnimatedComponent(Circle); export default function SvgExample() { const sv = useSharedValue('0%'); sv.value = withRepeat(withTiming('50%', { duration: 500 }), -1, true); return ( <View style={styles.container}> <Svg height="200" width="200"> <AnimatedCircle cx="50%" cy="50%" fill="lime" r={sv} /> </Svg> </View> ); } ``` This syntax is so pretty 🤩 but there is one issue with it though. We can't differentiate between a shared value and ordinary object with `value` prop. It may happen (though I think it's rather unlikely) that user doesn't want the prop to animate but just wants to pass an object or shared value. So I'm checking if it's a whitelisted prop (user had to whitelist it anyway). If that's not enough we may add whitelist/blacklist per component in `createAnimatedComponent`. Or use `animatedProps` prop but that won't be so pretty ;_;. Also I'm no adding a warning in babel plugin. Imo it would lead to too many false positives. Something like `<Component prop={obj.value} />` is normal. ## Test plan Example above. Run on ReanimatedExample, FabricExample and something similar on WebExample.
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please follow the template so that the reviewers can easily understand what the code changes affect. --> ## Summary - a rewrite of software-mansion#3354 - adds inline styles without animation functions just by starting a mapper - a mapper is started only when there are inline styles or inline styles have changed, also there is just one mapper for all styles properties for the given component - adds a way to disable inline styles warning in plugin options - adds support for transform property in styles, because I forgot to check that in the previous PR - it would be nice to have tests for this but `toHaveAnimatedStyle` won't work without any modifications (probably adding another updater just for jest testing like in `useAnimatedStyles`), so maybe I'll add it later ## Test plan Run this in ReanimatedExample, FabricExample and WebExample: <details> <summary>Example</summary> ```js import Animated, { useSharedValue, withTiming, Easing, } from 'react-native-reanimated'; import { View, Button } from 'react-native'; import React from 'react'; function AnimatedStyleUpdateExample(): React.ReactElement { const randomWidth = useSharedValue(10); const config = { duration: 500, easing: Easing.bezierFn(0.5, 0.01, 0, 1), }; return ( <View style={{ flex: 1, flexDirection: 'column', }}> <Animated.View style={[ { height: 80, backgroundColor: 'black', margin: 30 }, { width: randomWidth }, { transform: [{ translateX: randomWidth }] }, ]} /> <Button title="toggle" onPress={() => { randomWidth.value = withTiming(Math.random() * 350, config); }} /> </View> ); } export default AnimatedStyleUpdateExample; ``` </details>
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please follow the template so that the reviewers can easily understand what the code changes affect. --> ## Summary Based on software-mansion#4062 Adds support for inline animated props like this: ```js const AnimatedCircle = Animated.createAnimatedComponent(Circle); export default function SvgExample() { const sv = useSharedValue('0%'); sv.value = withRepeat(withTiming('50%', { duration: 500 }), -1, true); return ( <View style={styles.container}> <Svg height="200" width="200"> <AnimatedCircle cx="50%" cy="50%" fill="lime" r={sv} /> </Svg> </View> ); } ``` This syntax is so pretty 🤩 but there is one issue with it though. We can't differentiate between a shared value and ordinary object with `value` prop. It may happen (though I think it's rather unlikely) that user doesn't want the prop to animate but just wants to pass an object or shared value. So I'm checking if it's a whitelisted prop (user had to whitelist it anyway). If that's not enough we may add whitelist/blacklist per component in `createAnimatedComponent`. Or use `animatedProps` prop but that won't be so pretty ;_;. Also I'm no adding a warning in babel plugin. Imo it would lead to too many false positives. Something like `<Component prop={obj.value} />` is normal. ## Test plan Example above. Run on ReanimatedExample, FabricExample and something similar on WebExample.
Summary
toHaveAnimatedStyle
won't work without any modifications (probably adding another updater just for jest testing like inuseAnimatedStyles
), so maybe I'll add it laterTest plan
Run this in ReanimatedExample, FabricExample and WebExample:
Example