Improve UI props updating

[karol-bisztyga]

Description

The first goal of this PR was to fetch the name of the component for the prop updater what's needed on iOS. Until now we've provided RCTView for any component. Quoting the issue:

Where we always assume that we update props for RCTView component. Hence custom properties are added to some other view types, the props aren't properly applied and are filtered out

The component's name is obtained in createAnimatedComponent, on the javascript side, from the mounted component's props.

During creating animated component out of a regular one also UI props whitelist is being update dynamically(once for every unique component name detected). Thanks to this solution we are able to update props, specific for the certain components, on UI thread without hardcoding them.

Solves #1387

Reproducing

The issue occurs when there are some custom animated components which props are being updated on UI thread.
To reproduce you can use the following code:

code

import Animated, {
  useSharedValue,
  withTiming,
  useAnimatedStyle,
  useAnimatedProps,
} from 'react-native-reanimated';
import { View, Button } from 'react-native';
import React from 'react';
import { Svg, Circle, Rect, Ellipse } from 'react-native-svg';

export default function AnimatedStyleUpdateExample() {
  const ACircle = Animated.createAnimatedComponent(Circle);
  const ARect = Animated.createAnimatedComponent(Rect);
  const AEllipse = Animated.createAnimatedComponent(Ellipse);
  const randomWidth = useSharedValue(10);

  const style = useAnimatedStyle(() => {
    return {
      width: withTiming(randomWidth.value),
    };
  });

  const circleProps = useAnimatedProps(() => {
    return {
      fill: 'red',
      cx: 50,
      cy: 50,
      r: withTiming(randomWidth.value / 2),
    };
  });

  const ellipseProps = useAnimatedProps(() => {
    return {
      cx: 200,
      cy: 200,
      rx: withTiming(randomWidth.value / 4),
      ry: 200,
      stroke: 'black',
      strokeWidth: 2,
      fill: 'purple',
    };
  });

  const rectProps = useAnimatedProps(() => {
    return {
      x: withTiming(randomWidth.value / 4),
      y: 50,
      width: 50,
      height: 50,
      stroke: 'orange',
      strokeWidth: 2,
      fill: 'green',
    };
  });

  return (
    <View
      style={{
        flex: 1,
        flexDirection: 'column',
      }}>
      <Animated.View
        style={[
          { width: 100, height: 80, backgroundColor: 'black', margin: 30 },
          style,
        ]}
      />
      <Button
        title="toggle"
        onPress={() => {
          randomWidth.value = Math.random() * 350;
        }}
      />
      <Svg>
        <ACircle animatedProps={circleProps} />
        <ARect animatedProps={rectProps} />
        <AEllipse animatedProps={ellipseProps} />
      </Svg>
    </View>
  );
}

Also you have to add updated props to UI_THREAD_PROPS_WHITELIST in ConfigHelper.js, like this:

let UI_THREAD_PROPS_WHITELIST = {
  // ...
  rx: true,
  r: true,
}

If the view name in NativeProxy.mm is hardcoded to RCTView the props are not set but with proper view names they do work.

Testing

Some cases from svg library have been tested like rect, circle or ellipse as well as a regular RCTView.

Changes

Basically the main idea is placed in createAnimatedComponent where the view name is obtained(not directly but still) in componentDidMount so there's an access to a mounted component(and therefore to viewConfig). The code extending UI props list dynamically is placed in CodeHelper.js

[kmagiera]

I think this is a good direction overal. However I don't understand why we need so many places where we do some special handling specific to viewName that are not needed for viewTag which is already in the codebase. Seem to be like we should be able to treat these two attributes equally, is there any issue with that I may not see?

[karol-bisztyga]

I think this is a good direction overal. However I don't understand why we need so many places where we do some special handling specific to viewName that are not needed for viewTag which is already in the codebase. Seem to be like we should be able to treat these two attributes equally, is there any issue with that I may not see?

I made some changes regarding this.

Also, if someone wants to check how it works here goes a simple example based on AnimatedStyleUpdateExample:

code

import Animated, {
  useSharedValue,
  withTiming,
  useAnimatedStyle,
  Easing,
  useAnimatedProps,
} from 'react-native-reanimated';
import { View, Button } from 'react-native';
import React from 'react';
import { Svg, Circle, Rect, Ellipse, Text } from 'react-native-svg';

export default function AnimatedStyleUpdateExample() {

  const ACircle = Animated.createAnimatedComponent(Circle);
  const ARect = Animated.createAnimatedComponent(Rect);
  const AEllipse = Animated.createAnimatedComponent(Ellipse);
  const AText = Animated.createAnimatedComponent(Text);
  const randomWidth = useSharedValue(10);

  const config = {
    duration: 500,
    easing: Easing.bezier(0.5, 0.01, 0, 1),
  };

  const style = useAnimatedStyle(() => {
    return {
      width: withTiming(randomWidth.value, config),
    };
  });


  const props = useAnimatedProps(() => {
    return {
      fill: 'red',
      cx: 50,
      cy: 50,
      r: withTiming(randomWidth.value / 4),
    };
  });

  return (
    <View
      style={{
        flex: 1,
        flexDirection: 'column',
      }}>
      <Animated.View
        style={[
          { width: 100, height: 80, backgroundColor: 'black', margin: 30 },
          style,
        ]}
      />
      <Button
        title="toggle"
        onPress={() => {
          randomWidth.value = Math.random() * 350;
        }}
      />
      <Svg>
        <ACircle animatedProps={props} />
      </Svg>
      <ARect />
      <AEllipse />
      <AText />
      <Animated.Image />
      <Animated.Text />
    </View>
  );
}

[kmagiera]

Added a bunch of new comments.

On top of the comments this PR needs to include a test plan. As we discussed over the call, we need to make sure this has been tested in release builds as the internal fields we are accessing may work differently in dev vs release (not saying they do).

I looked at the sample code you posted in a comment – this should be a part of the PR description (maybe not the code, but at least a link to it). You should also mention what is the expected behavior. The code you posted works fine w/o this change, the difference is that the props are being updated via JS (with a roundtrip to JS and back), so it may appear to work fine while we expect it to work without doing that roundtrip.

[kmagiera]

under what condition the viewName will not be set? please at least add a comment here. IMO we should not have a fallback but rather throw but I don't know in what situations the name is not there

[karol-bisztyga]

This fails and takes that default value if _getViewName doesn't find corresponding view name. That shouldn't happen for react native and svg components. But I thought someone could try to create animated components out of some other external libraries(like svg) which in fact could do something what svg did which is putting component data in some custom place in a component's object(the svg's root case).

We could throw here - it's just a matter of how we want to handle such situation. On the other hand we could also fire up some warning like unsupported component type and alert the user that some props may not be set properly.

Mistakenly approved.

[karol-bisztyga]

Added a bunch of new comments.

On top of the comments this PR needs to include a test plan. As we discussed over the call, we need to make sure this has been tested in release builds as the internal fields we are accessing may work differently in dev vs release (not saying they do).

I looked at the sample code you posted in a comment – this should be a part of the PR description (maybe not the code, but at least a link to it). You should also mention what is the expected behavior. The code you posted works fine w/o this change, the difference is that the props are being updated via JS (with a roundtrip to JS and back), so it may appear to work fine while we expect it to work without doing that roundtrip.

I extended the description. The tests have been run for a several components from svg library on both debug and release modes.

[kmagiera]

Happy that you found a way to get native component instance and we no longer need to know implementation details of SVG or blur library to access the view name.

Can you also update PR name and description? I think after all the changes the current name/description is no longer relevant. Also would be great to include blurview in your testplan (that is, to make sure animating blur radius for example works on UI thread after this change)

[kmagiera]

I mentioned this earlier but can you please update PR title and description as the original title is no longer relevant. Otherwise the code looks ok