-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
feat: bump up fabric example react-native Android #3957
feat: bump up fabric example react-native Android #3957
Conversation
android/build.gradle
Outdated
@@ -117,8 +117,6 @@ android { | |||
exclude "**/libreact_render*.so" | |||
} | |||
|
|||
buildDir 'buildOutput_' + configStringPath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary really necessary ? The idea of this path configuration is to avoid cleaning project after enabling or disabling a build option...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's necessary for using New Architecture by react-native.
If I don't remove this buildDir 'buildOutput_' + configStringPath
settings, I get this error when I gradle sync in Android Studio.
I think react-native's settings are the main issue, but I think it would be easier to follow their pre-setup path for codegen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what is the main purpose of set up buildDir 'buildOutput_' + configStringPath
in build.gradle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use it in order to make "clean" build each time we change some android config (eg. extensions like IMA or exo-player)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I have a plan for resolving this issue.
If buildDir
is the only reason that we cannot merge this PR, my idea is like this.
First, revert this commit 62ec8d7
I think we can test Android Fabric without removing buildDir
in build.gradle.
And let's consider buildDir
later when Fabric implementation is almost done for Android.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed revert commit
Revert "feat: 🔥 turn off buildDir setting in android/build.gradle for…
Can you have a check my opinions? @freeboub
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sound good to me
btw now I understand why you want to remove it - I will do some research if we can "clean" build each time we change some android config in other way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KrzysztofMoch happy that you got my situation!
Your opinion would be another way to resolve this issue 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @freeboub
Can you check this thread please?
I think we don't need to delay about this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please check my comment, no issue with fabricExample update
… correct Fabric codegen dir location" This reverts commit 62ec8d7.
@freeboub I will merge this one as this is "only" example and change to |
Summary
FabricExample/android
for RN 0.73.2Motivation
Changes
coldsurfers:feat/rn-bump-up-fabric-example-ios
branch, so suggest review this PR after merge this PRbuildDir
settings in android/build.gradle for correct codegen location 62ec8d7Test plan
First, add this on package.json (root dir)
and paste this code fragment exactly same to
src/specs/VideoNativeComponent.ts
Lastly, before
build
orgradle sync
android of FabricExample,run next command to generate codegen for android in
FabricExample/android
directory../gradlew generateCodegenArtifactsFromSchema