microsoft / reactxp

Library for cross-platform app development.
https://microsoft.github.io/reactxp/
Other
8.29k stars 494 forks source link

RN 0.63 Button compatibility - useNativeDriver was not specified #1227

Open Frexuz opened 4 years ago

Frexuz commented 4 years ago

React Native now complains if useNativeDriver wasn't specified when animating.

Screenshot 2020-08-02 at 22 34 30

https://github.com/microsoft/reactxp/blob/3382b02e75f21b9af53cba888015b77d9392677c/src/native-common/Button.tsx#L372-L381

https://github.com/microsoft/reactxp/blob/3382b02e75f21b9af53cba888015b77d9392677c/src/native-common/Animated.tsx#L160-L167

I can PR, but not sure if it's safe to just add useNativeDriver: true?

deregtd commented 4 years ago

Not all animations can use native driver, so maybe we just need to force config.useNativeDriver to non-optional in typescript now.

fbartho commented 4 years ago

@deregtd that sounds like the right plan. -- I wish there was a cleaner API from react-native that would tell us if a particular Animation supports useNativeDriver. Then we could detect those cases and conditionally set the flag.

This feels like an API Wart from RN's side.

deregtd commented 4 years ago

API warts from react native are a significant portion of the reason ReactXP exists. :)

Frexuz commented 4 years ago

But the Button is only animating opacity in setOpacityTo. Why wouldn't this be ok?

 // reactxp/src/native-common/Button.tsx

 setOpacityTo(value: number, duration: number) { 
     Animated.timing( 
         this._opacityAnimatedValue!, 
         { 
             toValue: value, 
             duration: duration, 
             easing: Animated.Easing.InOut(), 
+            useNativeDriver: true, 
         }, 
     ).start(); 
 } 
deregtd commented 4 years ago

Ah, I misunderstood the suggestion. That change is okay, since the code appears to only animate opacity specifically in the button. But also we need to change the config's type to be nonoptional now to find any other broken spots.

fbartho commented 4 years ago

@Frexuz -- I think we should do this. Do you want to submit a PR for it?

Frexuz commented 4 years ago

@fbartho yep :)