fix: new arch measurements on Android #589

okwasniewski · 2024-03-27T13:07:07Z

Summary:

This PR adds C++ shadow nodes to implement proper measuring for Android on Fabric/Bridgeless.

This code was based on Core implementation: https://github.com/facebook/react-native/blob/2af1da42ff517232f1309efed7565fe9ddbbac77/packages/react-native/ReactCommon/react/renderer/components/switch/androidswitch/react/renderer/components/androidswitch/AndroidSwitchShadowNode.cpp

Before

After

Test Plan:

Check if on new architecture Slider is properly rendered without any styles

thymikee · 2024-03-28T13:09:57Z

That's quite a lot of boilerplate to make it work. Is this expected in this case @cortinico?

okwasniewski · 2024-03-28T14:54:23Z

package/android/src/newarch/java/com/reactnativecommunity/slider/ReactSliderManager.java

+    public Map<String, Object> getExportedCustomBubblingEventTypeConstants() {
+        return ReactSliderManagerImpl.getExportedCustomBubblingEventTypeConstants();
+    }
+
+    @Nullable
+    @Override
+    public Map<String, Object> getExportedCustomDirectEventTypeConstants() {
+        return ReactSliderManagerImpl.getExportedCustomDirectEventTypeConstants();
+    }


This now properly exports bubbling and direct events according to the JS spec

cortinico · 2024-03-28T16:46:50Z

package/src/RNCSliderNativeComponent.ts

-  'RNCSlider',
-) as HostComponent<NativeProps>;
+export default codegenNativeComponent<NativeProps>('RNCSlider', {
+  interfaceOnly: true,


Q: What's the reason you don't use the generate C++ code and expand from it?

So when I removed this I got a new issue saying that I'm redefining ShadowNode with different types - how can I achieve what you mentioned to expand from generated C++ code?

Full Error message

/Users/okwasniewski/Library/Android/sdk/ndk/26.1.10909125/toolchains/llvm/prebuilt/darwin-x86_64/bin/clang++ --target=aarch64-none-linux-android23 --sysroot=/Users/okwasniewski/Library/Android/sdk/ndk/26.1.10909125/toolchains/llvm/prebuilt/darwin-x86_64/sysroot -Dappmodules_EXPORTS -I/Users/okwasniewski/workspace/react-native-slider/example/node_modules/react-native/ReactAndroid/cmake-utils/default-app-setup -I/Users/okwasniewski/workspace/react-native-slider/example/android/app/build/generated/rncli/src/main/jni -I/Users/okwasniewski/workspace/react-native-slider/example/node_modules/@react-native-community/slider/android/src/main/jni -I/Users/okwasniewski/workspace/react-native-slider/example/node_modules/@react-native-community/slider/android/src/main/jni/. -I/Users/okwasniewski/workspace/react-native-slider/example/node_modules/@react-native-community/slider/android/src/main/jni/../../../../common/cpp -I/Users/okwasniewski/workspace/react-native-slider/example/node_modules/@react-native-community/slider/android/src/main/jni/../../../build/generated/source/codegen/jni -I/Users/okwasniewski/workspace/react-native-slider/example/node_modules/@react-native-community/slider/android/src/main/jni/../../../build/generated/source/codegen/jni/react/renderer/components/RNCSlider -I/Users/okwasniewski/workspace/react-native-slider/example/node_modules/react-native-pager-view/android/build/generated/source/codegen/jni/. -I/Users/okwasniewski/workspace/react-native-slider/example/node_modules/react-native-pager-view/android/build/generated/source/codegen/jni/react/renderer/components/RNCViewPager -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/fabricjni/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/c7e26f7d3c2fc1a6636ff1467fc77795/transformed/jetified-fbjni-0.6.0/prefab/modules/fbjni/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/folly_runtime/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/glog/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/jsi/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/react_codegen_rncore/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/react_debug/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/react_utils/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/react_nativemodule_core/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/react_newarchdefaults/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/react_cxxreactpackage/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/react_render_componentregistry/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/react_render_core/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/react_render_debug/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/react_render_graphics/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/react_render_imagemanager/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/react_render_mapbuffer/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/react_render_textlayoutmanager/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/rrc_image/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/rrc_view/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/rrc_text/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/rrc_textinput/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/rrc_legacyviewmanagerinterop/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/runtimeexecutor/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/turbomodulejsijni/include -isystem /Users/okwasniewski/.gradle/caches/transforms-4/633fb3b736b7e99b53766369b7cb59e4/transformed/jetified-react-android-0.74.0-rc.5-debug/prefab/modules/yoga/include -g -DANDROID -fdata-sections -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security -fno-limit-debug-info -fPIC -Wall -Werror -Wno-error=cpp -fexceptions -frtti -std=c++20 -DLOG_TAG=\"ReactNative\" -DFOLLY_NO_CONFIG=1 -DFOLLY_HAVE_CLOCK_GETTIME=1 -DFOLLY_USE_LIBCPP=1 -DFOLLY_CFG_NO_COROUTINES=1 -DFOLLY_MOBILE=1 -DFOLLY_HAVE_RECVMMSG=1 -DFOLLY_HAVE_PTHREAD=1 -DFOLLY_HAVE_XSI_STRERROR_R=1 -MD -MT CMakeFiles/appmodules.dir/Users/okwasniewski/workspace/react-native-slider/example/android/app/build/generated/rncli/src/main/jni/rncli.cpp.o -MF CMakeFiles/appmodules.dir/Users/okwasniewski/workspace/react-native-slider/example/android/app/build/generated/rncli/src/main/jni/rncli.cpp.o.d -o CMakeFiles/appmodules.dir/Users/okwasniewski/workspace/react-native-slider/example/android/app/build/generated/rncli/src/main/jni/rncli.cpp.o -c /Users/okwasniewski/workspace/react-native-slider/example/android/app/build/generated/rncli/src/main/jni/rncli.cpp In file included from /Users/okwasniewski/workspace/react-native-slider/example/android/app/build/generated/rncli/src/main/jni/rncli.cpp:11: In file included from /Users/okwasniewski/workspace/react-native-slider/example/node_modules/@react-native-community/slider/android/src/main/jni/../../../build/generated/source/codegen/jni/react/renderer/components/RNCSlider/ComponentDescriptors.h:13: /Users/okwasniewski/workspace/react-native-slider/example/node_modules/@react-native-community/slider/android/src/main/jni/../../../build/generated/source/codegen/jni/react/renderer/components/RNCSlider/ShadowNodes.h:26:7: error: typedef redefinition with different types ('ConcreteViewShadowNode<RNCSliderComponentName, RNCSliderProps, RNCSliderEventEmitter, RNCSliderState>' vs 'facebook::react::RNCSliderShadowNode') using RNCSliderShadowNode = ConcreteViewShadowNode< ^ /Users/okwasniewski/workspace/react-native-slider/example/node_modules/@react-native-community/slider/android/src/main/jni/../../../../common/cpp/react/renderer/components/RNCSlider/RNCSliderShadowNode.h:20:26: note: previous definition is here class JSI_EXPORT RNCSliderShadowNode final ^

Ok so it's a bit more complicated. Are you fine with merging this as it is, as it unblocks New Architecture migration, and then we can follow-up on a cleanup afterwards?

cortinico · 2024-03-28T16:48:15Z

That's quite a lot of boilerplate to make it work. Is this expected in this case @cortinico?

In theory you could use codegen and pick only the C++ files you need

okwasniewski · 2024-04-02T10:32:06Z

Hey, @BartoszKlonowski could you check if this fails locally for you on iOS? For me it works fine but CI/CD fails for some reason

BartoszKlonowski · 2024-04-02T13:08:45Z

Hey, @BartoszKlonowski could you check if this fails locally for you on iOS? For me it works fine but CI/CD fails for some reason

The issue was that we first run the codegen but only then we pull the package with the current changes from the PR, so in this case the changes that required the codegen to apply were made after the codegen, hence some sources/files couldn't be found (as they didn't have chance to be generated).
Changing the order of the steps so that we first pull the latest package from PR and then run codegen fixed it.

This was definitely an issue in general, but looks like previously we didn't have a chance to discover it. I'm glad we had that opportunity in this PR.

Thank you for notifying and pinging me! 👍

cortinico · 2024-04-02T16:01:30Z

package/src/RNCSliderNativeComponent.ts

-  'RNCSlider',
-) as HostComponent<NativeProps>;
+export default codegenNativeComponent<NativeProps>('RNCSlider', {
+  interfaceOnly: true,


Ok so it's a bit more complicated. Are you fine with merging this as it is, as it unblocks New Architecture migration, and then we can follow-up on a cleanup afterwards?

okwasniewski · 2024-04-03T06:33:30Z

Yup, I'm fine with merging this as is and clean it up later if we come up with better solutions in core. WDYT @thymikee @BartoszKlonowski ?

BartoszKlonowski

Let's have that, but we will need to create an issue to mark the cleanup.
I also left a couple of comments, but they can be addressed later too.

package/common/cpp/react/renderer/components/RNCSlider/RNCSliderMeasurementsManager.h

package/common/cpp/react/renderer/components/RNCSlider/RNCSliderShadowNode.cpp

package/common/cpp/react/renderer/components/RNCSlider/RNCSliderState.cpp

okwasniewski · 2024-04-04T09:09:53Z

@BartoszKlonowski I've applied your review comments

okwasniewski force-pushed the fix/new-arch-measurements branch 2 times, most recently from 7ad892b to 7d056cc Compare March 28, 2024 10:35

okwasniewski changed the title ~~[DRAFT] fix: new arch measurements~~ fix: new arch measurements on Android Mar 28, 2024

okwasniewski force-pushed the fix/new-arch-measurements branch 2 times, most recently from 9dbb2f9 to b801550 Compare March 28, 2024 14:53

okwasniewski commented Mar 28, 2024

View reviewed changes

okwasniewski marked this pull request as ready for review March 28, 2024 14:55

cortinico reviewed Mar 28, 2024

View reviewed changes

okwasniewski force-pushed the fix/new-arch-measurements branch 8 times, most recently from afb04ed to d629ea9 Compare April 2, 2024 09:26

fix: new arch measurements

182a80c

okwasniewski force-pushed the fix/new-arch-measurements branch from d629ea9 to 182a80c Compare April 2, 2024 09:55

Use the current package sources before installing pods

6d04c80

okwasniewski requested a review from cortinico April 2, 2024 13:28

cortinico approved these changes Apr 2, 2024

View reviewed changes

BartoszKlonowski approved these changes Apr 3, 2024

View reviewed changes

fix: adjust review comments

67431e4

BartoszKlonowski merged commit 5397010 into main Apr 4, 2024
8 checks passed

BartoszKlonowski mentioned this pull request Apr 11, 2024

Fix missing header guard #592

Merged

cortinico mentioned this pull request Apr 20, 2024

Use ReactContext instead of ThemedReactContext in PaywallFooterViewManager RevenueCat/react-native-purchases#971

Merged

vonovak mentioned this pull request Oct 4, 2024

slider has 0 height on Android with new architecture enabled #652

Closed

okwasniewski mentioned this pull request Oct 21, 2024

[WIP] new arch on android callstackincubator/react-native-bottom-tabs#68

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: new arch measurements on Android #589

fix: new arch measurements on Android #589

okwasniewski commented Mar 27, 2024 •

edited

Loading

thymikee commented Mar 28, 2024

okwasniewski Mar 28, 2024

cortinico Mar 28, 2024

okwasniewski Mar 29, 2024 •

edited

Loading

cortinico Apr 2, 2024

cortinico commented Mar 28, 2024

okwasniewski commented Apr 2, 2024 •

edited

Loading

BartoszKlonowski commented Apr 2, 2024 •

edited

Loading

cortinico Apr 2, 2024

okwasniewski commented Apr 3, 2024

BartoszKlonowski left a comment

okwasniewski commented Apr 4, 2024

fix: new arch measurements on Android #589

fix: new arch measurements on Android #589

Conversation

okwasniewski commented Mar 27, 2024 • edited Loading

Summary:

Before

After

Test Plan:

thymikee commented Mar 28, 2024

okwasniewski Mar 28, 2024

Choose a reason for hiding this comment

cortinico Mar 28, 2024

Choose a reason for hiding this comment

okwasniewski Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

cortinico Apr 2, 2024

Choose a reason for hiding this comment

cortinico commented Mar 28, 2024

okwasniewski commented Apr 2, 2024 • edited Loading

BartoszKlonowski commented Apr 2, 2024 • edited Loading

cortinico Apr 2, 2024

Choose a reason for hiding this comment

okwasniewski commented Apr 3, 2024

BartoszKlonowski left a comment

Choose a reason for hiding this comment

okwasniewski commented Apr 4, 2024

okwasniewski commented Mar 27, 2024 •

edited

Loading

okwasniewski Mar 29, 2024 •

edited

Loading

okwasniewski commented Apr 2, 2024 •

edited

Loading

BartoszKlonowski commented Apr 2, 2024 •

edited

Loading