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

make reanimated work in web #1886

Merged
merged 8 commits into from
Oct 13, 2022

Conversation

phryneas
Copy link
Contributor

Summary

Up until now, trying to use reanimated with react-native-svg in react-native-web resulted in an error.
This adds a setNativeProps function to the web implementation to directly modify the transform and style props on a SVGElement ref.

Since there is a need to track the "last merged props" and those need to be reset on every render, the render method has been moved into the WebShape class and a tag string property has been added.

As g had some extra handling for x and y, a prepareProps function was added as well.

Test Plan

Here is a video of me dragging a <g> with a nested <image> in chome: https://www.loom.com/share/675405537d9d41a792432ebf41636047

What's required for testing (prerequisites)?

Just run any reanimated svg in the browser instead of a device.

What are the steps to reproduce (after prerequisites)?

Run any animation.

Compatibility

OS Implemented
iOS (untouched)
Android (untouched)
Web

Checklist

  • I have tested this on a device and a simulator
  • I have tested this on a web browser
  • I added documentation in README.md
  • I updated the typed files (typescript)
  • I added a test for the API in the __tests__ folder

@necolas
Copy link

necolas commented Sep 29, 2022

FYI setNativeProps will not be supported in the new RN architecture and is being removed from RNWeb too

@phryneas
Copy link
Contributor Author

@necolas good point.
It looks like I could take a look into exposing a _touchableNode with a setAttribute function as an alternative.

Just could probably not be a direct exposure of the DOM ref since they set all attributes in sequence and we'd need all of the new props to calculate a new transform string.

@WoLewicki
Copy link
Member

Could you add some examples to e.g. Example project which would show these changes?

@phryneas
Copy link
Contributor Author

@WoLewicki I can give it a spin.

By the way, I'm just going at that _touchableNode part and stumbled across software-mansion/react-native-reanimated#3321. The solution I'm trying right now might also fix that.

@phryneas
Copy link
Contributor Author

and I added an example :)

@phryneas
Copy link
Contributor Author

phryneas commented Oct 1, 2022

Also, maybe I should add some explanation on what this is doing and how it is interacting.

From the users perspective:

  • usually a component like an react-native-svg-component would be wrapped in a createReanimatedComponent HOC that would add an animatedProps property
  • then the user would use the useAnimatedProps hook to create an object to pass into that animatedProps property.
  • changing shared values mapped that way would now not cause a component rerender, but internally change properties on the rendered component

This is the example from the docs

import React from 'react';
import { StyleSheet } from 'react-native';
import Animated, {
  useSharedValue,
  useAnimatedProps,
} from 'react-native-reanimated';
import Svg, { Path } from 'react-native-svg';

const AnimatedPath = Animated.createAnimatedComponent(Path);

function App() {
  const radius = useSharedValue(50);

  const animatedProps = useAnimatedProps(() => {
    // draw a circle
    const path = `
    M 100, 100
    m -${radius.value}, 0
    a ${radius.value},${radius.value} 0 1,0 ${radius.value * 2},0
    a ${radius.value},${radius.value} 0 1,0 ${-radius.value * 2},0
    `;
    return {
      d: path,
    };
  });

  // attach animated props to an SVG path using animatedProps
  return (
    <Svg>
      <AnimatedPath animatedProps={animatedProps} fill="black" />
    </Svg>
  );
}

Now, let's look at the react-native-reanimated internals of those "off-render-updates":

    if (typeof component.setNativeProps === 'function') {
      setNativeProps(component, rawStyles);
    } else if (Object.keys(component.props).length > 0) {
      Object.keys(component.props).forEach((key) => {
        if (!rawStyles[key]) {
          return;
        }
        const dashedKey = key.replace(/[A-Z]/g, (m) => '-' + m.toLowerCase());
        component._touchableNode.setAttribute(dashedKey, rawStyles[key]);
      });
    } else {
      console.warn('It is not possible to manipulate component');
    }

The relevant part:

  • if there is a setNativeProps function on the component wrapped, that will be called with the whole updated prop object at once. This was my first approach, but it seems setNativeProps is deprecated now.
  • if that is not present, reanimated will instead iterate through all attributes, will convert camelCase to dashed keys and set each attribute individually.

Now, "setting each attribute individually" faces two problems from the side of react-native-svg:

  • with react-native-svg, those cannot be set directly on the DOM element as there is that prepare function taking all those properties and converting them to a string transform property
  • since all of those are combined into one string, they also have to be set at once and not bit-by-bit

Hence, my approach here:

  • make sure that there is a component._touchableNode property in the first place (up until now, that was only present through the Touchable Mixin, which was only added when the user added touch listeners
  • wrap component._touchableNode.setAttribute to our own logic - I used a proxy on the original DOM node here to override only this one specific property
  • "batch" modifications coming in from the outside before building a new transform string that will be applied - I do this by scheduling an update with requestAnimationFrame, which will make sure that any synchronous calls like the for loop of reanimated above will have ended before the batch is executed
  • if meanwhile a render happens, cancel the batched update and reset the batched properties - the latest render is sure to have the most up-to-date data.

I hope this explains most of my thought process behind this PR :)

PS: All that said, there might be merit to start a discussion over in https://github.com/software-mansion/react-native-reanimated if it might not be possible to add a [reanimatedUpdateAttributesSymbol] property to a component that would be called with a full set of updated attributes by reanimated directly, so there is a direct integration between reanimated and supporting libraries, but I guess something like that would take a very long time to land & spread through the ecosystem, if accepted at all.

PS2: I opened a discussion that you can find at software-mansion/react-native-reanimated#3633, but not only adding this for web would require quite deep changes.

@WoLewicki
Copy link
Member

So would it be possible to rather change how reanimated's web code works instead of those hacky solutions on the react-native-svg side? It seems like this PR changes quite a lot in the web implementation which might break other behaviors.

@phryneas
Copy link
Contributor Author

phryneas commented Oct 4, 2022

@WoLewicki assuming react-native-reanimated would change their internals (and quite a lot of them), yes. Otherwise, no.

To be honest, this PR doesn't really change a lot behaviour that was there before.

Moving the render method into the parent class should not have a chance of breaking anything (this is the biggest "code change").

The only thing that is changed on the outside interface is that calling refToComponent._touchableNode.setAttribute will not directly set DOM attributes, but runs those with a minimal delay through the clean function.
Realistically, this would have been buggy behaviour in many cases before and should now work more like intended.

Benefit of getting this PR in is that this would also mean it probably works with different animation implementations that call _touchableNode.setAttribute (tbh., I don't know if those exist though, I'm quite new to the ecosystem).

@WoLewicki
Copy link
Member

After consultation with @kmagiera we came to a conclusion that the first version of this PR using setNativeProps seems like a better option for now. react-native-reanimated still uses setNativeProps on web, so since this PR brings integration with react-native-reanimated, it should be fine to use it. Could you revert the changes then?

@phryneas
Copy link
Contributor Author

@WoLewicki sure, I reverted those files.
Should it ever be necessary to go the other route, I will keep a branch around with those changes at https://github.com/phryneas/react-native-svg/tree/reanimated-touchableNode

@phryneas
Copy link
Contributor Author

Seems these CI errors come from the example app, or rather from the dependency to reanimated.
Quick solution would be to turn skipLibCheck to false - would you be okay with that?

@WoLewicki
Copy link
Member

Yeah, it seems right to not check other libraries' types in PRs for this library 😅

@WoLewicki
Copy link
Member

I checked the current implementation and the rectangle seems to be stuck in one place, not animating. Also, could you add a fill prop to it? On iOS and Android this rectangle seems to be white by default so it cannot be seen.

@phryneas
Copy link
Contributor Author

I've changed it to red, so it's visible on Android as well (I don't have an iOS device to check at hand).

For me, the rectangle is animating in both Chrome and Firefox. I'm not really sure what's going on there.. :/
Maybe some build cache issues?

@WoLewicki
Copy link
Member

It is frozen at the beginning for me both on chrome and safari. I tested it on the fresh repo so I am not sure what can be a problem here:
image

@phryneas
Copy link
Contributor Author

Hmm, I can reproduce that on a different machine. Maybe I am the one with the stale cache. I'll look into it.

@phryneas
Copy link
Contributor Author

Got it. The original code only applied changes to transform and style, both of which were not affected at all by the example. I had fixed that in the _touchableNode version of this PR, but not in the initial one.
Now it should animate.

Copy link
Member

@WoLewicki WoLewicki left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your contribution 🎉

@phryneas
Copy link
Contributor Author

Lol last minute fix, I'm sorry!

@phryneas
Copy link
Contributor Author

I noticed that style could also come in as an array, so I handle that case now, too.

@WoLewicki
Copy link
Member

Ok, could you also run pod install in the Example app since you added reanimated to the project? It should change the Podfile.lock.

@phryneas
Copy link
Contributor Author

Ok, could you also run pod install in the Example app since you added reanimated to the project? It should change the Podfile.lock.

I'm on a Linux machine, so I don't have pod available, I'm sorry.

Also, why this is needed: #1886 (files) ?

To assign properties not on style, like transform, x etc.
When reanimated calls these updates directly on a DOM node, they replace camelCase with dashed attribute names first, so I just replicated their logic there.
Relevant code: https://github.com/software-mansion/react-native-reanimated/blob/d04720c82f5941532991b235787285d36d717247/src/reanimated2/js-reanimated/index.ts#L34-L40

@WoLewicki
Copy link
Member

Ok so could you please add a comment in code with the link to the relevant code you just linked?

@phryneas
Copy link
Contributor Author

Sure :) Are you happy with it like that?

@WoLewicki
Copy link
Member

Yeah, looks good now 🚀

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.

3 participants