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

Add inline styles without animation functions #4062

Merged
merged 18 commits into from
Feb 22, 2023

Conversation

graszka22
Copy link
Member

Summary

  • a rewrite of Add inline styles #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:

Example
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;

Copy link
Member

@kmagiera kmagiera left a 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 =
Copy link
Member

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
Copy link
Member

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?

const newStyle: StyleProps = {};
for (const [key, styleValue] of Object.entries(style)) {
if (
styleValue.value === undefined &&
Copy link
Member

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;
}
Copy link
Member

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

Comment on lines 495 to 498
update[key] = styleValue.map((t: Record<string, any>) =>
Object.keys(t).reduce(
(acc, curr) => ({ ...acc, [curr]: t[curr].value }),
{}
Copy link
Member

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

Copy link
Member

@kmagiera kmagiera left a 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


export function isSharedValue(obj: any) {
'worklet';
return typeof obj === 'object' && obj.value !== undefined;
Copy link
Member

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(
Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I used the function to get the warning message but left the console.warn call in the plugin. That way we still get a nice arrow in Source section that shows exactly where is the error.

Screenshot 2023-02-21 at 16 51 03

When console.warn is in the function we'd get something like that:

Screenshot 2023-02-21 at 16 40 06

if (Object.keys(newInlineStyles).length) {
this._inlineStylesMapperId = startMapper(
updaterFunction,
Object.values(newInlineStyles)
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

@kmagiera kmagiera left a 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;
Copy link
Member

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.

Copy link
Member

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

@graszka22 graszka22 added this pull request to the merge queue Feb 22, 2023
Merged via the queue into main with commit a073f21 Feb 22, 2023
@graszka22 graszka22 deleted the graszka22/sv-only-inline-styles branch February 22, 2023 02:36
graszka22 added a commit that referenced this pull request Feb 22, 2023
<!-- 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.
tjzel added a commit that referenced this pull request Feb 27, 2023
fluiddot pushed a commit to wordpress-mobile/react-native-reanimated that referenced this pull request Jun 5, 2023
<!-- 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>
fluiddot pushed a commit to wordpress-mobile/react-native-reanimated that referenced this pull request Jun 5, 2023
<!-- 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.
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.

2 participants