feat: support Fabric #1754
feat: support Fabric #1754
Conversation
|
Hi guys, thank you for working on this pr |
|
@AlirezaHadjar thanks for kind words. It is unfortunately very hard to estimate the time needed to merge it since there are many components which need to be transferred and also there can be many implementation details in Fabric which we are not aware of at the moment that can slow down the process. |
|
@WoLewicki Could i help you doing anything ? |
|
@mateosilguero thanks for wanting to contribute! One thing I can think of at the moment is a research for the ability to pass |
|
Hi @WoLewicki , sorry to bother but there’s no plans to continue this implementation in a near future? |
|
@sebasg0 I ended fixing Fabric issues in |
|
@WoLewicki good news |
|
Hi 👋 I added a second PR, pointing to this branch, with most of Android changes here: #1804. I think it is now ready to review, for sure there are many things that should be done better. Please test it and submit any bug you encounter 🚀 |
| */ | ||
|
|
||
| project.ext.react = [ | ||
| enableHermes: false, // clean and rebuild if changing |
There was a problem hiding this comment.
Shouldn't Hermes be enabled? (It's not changed in the Android PR).
There was a problem hiding this comment.
Hmm, it is not enabled in react-native-screens: https://github.com/software-mansion/react-native-screens/blob/7e4a348bd360ecc5aa7ba378a8adbdc3dff9fc37/FabricTestExample/android/app/build.gradle#L81, do you think it would change anything regarding potential bugs?
There was a problem hiding this comment.
I'm not sure, In the Sample new arch app it's one of the steps to enable it, but if it works here and in react-native-screens then it's fine, I guess?
There was a problem hiding this comment.
I'll enable it as it is a recommended step 👍
| <Text>TEST</Text> | ||
| </> | ||
| ); | ||
| } |
| }, | ||
| }), | ||
| }, | ||
| }; |
| { | ||
| "name": "FabricExample", | ||
| "displayName": "FabricExample" | ||
| } |
| static SVGLength from(double number) { | ||
| return new SVGLength(number); | ||
| } |
There was a problem hiding this comment.
It looks like indentation is wrong here.
There was a problem hiding this comment.
Yeah, I will have to apply some kind of formatter for JS, Java and iOS files.
| return facebook::jni::initialize(vm, [] { | ||
| facebook::react::RNSvgComponentsRegistry::registerNatives(); | ||
| }); | ||
| } |
| }; | ||
|
|
||
| } // namespace react | ||
| } // namespace facebook |
|
|
||
| type ComponentType = HostComponent<NativeProps>; | ||
|
|
||
| export default (codegenNativeComponent<NativeProps>('RNSVGCircle', {}): ComponentType); |
There was a problem hiding this comment.
New line here and in every other *NativeComponent.js file 🙂
| @@ -151,6 +151,17 @@ export function extract(instance: Object, props: Object & { style?: [] | {} }) { | |||
| return extractProps(propsAndStyles(props), instance); | |||
| } | |||
|
|
|||
| export function stringifyPropsForFabric(props: { [key: string]: NumberProp | undefined | null; }): { [key: string]: string; } { | |||
There was a problem hiding this comment.
It looks like this function is beeing called regardless of whether Paper or Fabric is used. Are the props it's generating used only on Fabric or on Paper too?
There was a problem hiding this comment.
For now the implementation makes the change of all NumberProp props to string on both archs. I would prefer not to do it but I cannot find a way around it for now. I could add checks for Fabric in all of those places if we don't find another way to handle it.
|
You can check #1821 for the version of this PR without |
|
It would be great to get this merged. I am migrating my apps to new arch. Thanks to all the contributors :) |
Version of #1754 without usage of ComponentViews. It seems like a more proper way, but introduces the necessity of clearing whole state of each component on recycling for it not to be used when view is recycled. Still known problems: We stringify props of type NumberProp since codegen only accepts props of a single type. It is the fastest way of dealing with it, but maybe there could be a better way to handle it. Image resolving should be probably handled the same as in RN core SvgView needs to set opaque = NO so it is does not have black background (it comes from the fact that RCTViewComponentView overrides backgroundColor setter which in turn somehow messes with the view being opaque). All other svg components do it already so maybe it is not such an issue. transform prop won't work when set natively since it is not parsed in Fabric
|
Great work y'all. Any chance we can get this merged please? Would love to test it out 🙏 |
Version of software-mansion#1754 without usage of ComponentViews. It seems like a more proper way, but introduces the necessity of clearing whole state of each component on recycling for it not to be used when view is recycled. Still known problems: We stringify props of type NumberProp since codegen only accepts props of a single type. It is the fastest way of dealing with it, but maybe there could be a better way to handle it. Image resolving should be probably handled the same as in RN core SvgView needs to set opaque = NO so it is does not have black background (it comes from the fact that RCTViewComponentView overrides backgroundColor setter which in turn somehow messes with the view being opaque). All other svg components do it already so maybe it is not such an issue. transform prop won't work when set natively since it is not parsed in Fabric
Version of software-mansion#1754 without usage of ComponentViews. It seems like a more proper way, but introduces the necessity of clearing whole state of each component on recycling for it not to be used when view is recycled. Still known problems: We stringify props of type NumberProp since codegen only accepts props of a single type. It is the fastest way of dealing with it, but maybe there could be a better way to handle it. Image resolving should be probably handled the same as in RN core SvgView needs to set opaque = NO so it is does not have black background (it comes from the fact that RCTViewComponentView overrides backgroundColor setter which in turn somehow messes with the view being opaque). All other svg components do it already so maybe it is not such an issue. transform prop won't work when set natively since it is not parsed in Fabric
PR adding support for Fabric to the library. See #1804 for the working version for both platforms.
One of the main changes that can affect old architecture is stringifying all props of type
NumberPropsince codegen does not allow for props to have typestringornumber. It would be best to find a way around it since it might cause issues e.g. in libraries likereact-native-reanimatedwhere we want to skip therenderpart and change props directly on the native side. It seems not to be problematic with RNAnimatedsince on FabricsetNativePropsis not supported so probably all changes even withnativeDriverare going through arenderbut I might be wrong.Another one is
iOSview hierarchy structure. Right now allComponentViews have their corresponding svg elements ascontentViews. ThemountChildComponentViewandunmountChildComponentViewmethods of thoseComponentViews have been changed such that the views are added directly as children ofcontentViews of their parents so we keep the view structure from the old architecture. It was working well, but recently I discovered that whenComponentViews are not present in the native view hierarchy, the responder system stopped working. Due to this, I also addComponentViews in a different view hierarchy just so they are present there, making the responder system work correctly.