-
Notifications
You must be signed in to change notification settings - Fork 384
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
@W-13557285: [Android] Upgrade to Gradle 8.x and AGP 8.x Via RN 0.72.x #2492
@W-13557285: [Android] Upgrade to Gradle 8.x and AGP 8.x Via RN 0.72.x #2492
Conversation
a83d12b
to
b82742e
Compare
Tests results for SalesforceSDKGenerated by 🚫 Danger |
…ndroidX Core KTX 1.9.0)
1209a28
to
ff75ebe
Compare
This is looking good! |
f7aa2e6
to
ad778f3
Compare
…eact Tests Corrections)
ad778f3
to
ffbf86c
Compare
…ndroidX Core KTX 1.12.0)
@@ -9,7 +9,7 @@ buildscript { | |||
} | |||
|
|||
dependencies { | |||
classpath("com.android.tools.build:gradle:7.4.2") | |||
classpath("com.android.tools.build:gradle:8.2.0") |
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.
A crux change is updating to Android Gradle Plugin 8.2.0, of course.
implementation("org.jetbrains.kotlin:kotlin-gradle-plugin:1.9.20") | ||
implementation("org.jetbrains.kotlin:kotlin-stdlib:1.8.22") | ||
implementation("org.jetbrains.kotlin:kotlin-stdlib:1.9.21") |
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.
A Kotlin standard library 1.9.21 update comes along with the update to Gradle 8.2 and Android Gradle Plugin 8.2.0. If I recall the testing correctly, this aids downstream in avoiding the template repository's issues in using the new Android core-ktx 1.12.0 version.
@@ -1,5 +1,5 @@ | |||
distributionBase=GRADLE_USER_HOME | |||
distributionPath=wrapper/dists | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-7.6-bin.zip | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-8.2-bin.zip |
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.
Another crux change: Updating to Gradle 8.2.
@@ -76,3 +76,7 @@ android { | |||
aidl = true | |||
} | |||
} | |||
|
|||
kotlin { | |||
jvmToolchain(17) |
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.
The update to use JVM toolchain was recommended by Gradle, but I only applied it specifically where Gradle suggested it.
@@ -15,7 +15,7 @@ dependencies { | |||
api("org.apache.cordova:framework:12.0.1") | |||
api("androidx.appcompat:appcompat:1.6.1") | |||
api("androidx.appcompat:appcompat-resources:1.6.1") | |||
api("androidx.webkit:webkit:1.8.0") | |||
api("androidx.webkit:webkit:1.9.0") |
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.
This is a rider update, but I thought it applicable since we just did a full update to all dependencies for 12.0.0 on dev
.
@@ -23,7 +23,7 @@ plugins { | |||
|
|||
dependencies { | |||
api(project(":libs:MobileSync")) | |||
api("com.facebook.react:react-native:0.70.14") | |||
api("com.facebook.react:react-android:0.72.7") |
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.
This is a specific dependency change from the React Native upgrade helper, as shown in the template repository apps. However, note here a specific version is used since we seem to be unable to apply the React Native Gradle plugin to our library module. If anyone knows of React Native documentation specific to integrating it with non-app modules, let me know since I wasn't able to find it.
@@ -137,12 +137,9 @@ task<Exec>("buildReactTestBundle") { | |||
} | |||
|
|||
task("buildReactTestBundleIfNotExists") { | |||
doLast { |
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.
This doLast
specifically fails in Gradle/AGP 8. Everything does seem to test correctly without it, however. I'd be game to know more if anyone recalls why this was so.
const {getDefaultConfig, mergeConfig} = require('@react-native/metro-config'); | ||
|
||
/** | ||
* Metro configuration for React Native |
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 do have a start
command in this module's package.json
, so this new file is required to use that. However, when would we start this React Native module? If that's not used, the command and this file could be removed.
"scripts": { | ||
"start": "node node_modules/react-native/local-cli/cli.js start" | ||
}, | ||
"dependencies": { |
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 used ncu
to update the entire dependency list to current specs, which really seemed to help get a consistent configuration when compared to manually updating individual dependencies.
libs/SalesforceReact/package.json
Outdated
"create-react-class": "^15.7.0", | ||
"react": "18.2.0", | ||
"react-native": "0.72.7", | ||
"react-native-force": "git+https://github.com/JohnsonEricAtSalesforce/SalesforceMobileSDK-ReactNative.git#feature/w-13557285-gradle-8-react-native-0.72.6" |
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 am still waiting to update the related repository paths until I do final self review on all four repos.
@@ -42,11 +42,10 @@ | |||
|
|||
/** | |||
* Subclass of ReactNativeHost used for testing | |||
* | |||
* <p> |
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.
Resolves a cosmetic inspector warning.
* In addition to the SalesforceReact react packages, it loads SalesforceReactTestPackage (which brings SalesforceTestBridge) | ||
* It creates an ReactInstanceManager which handles error through NativeModuleCallExceptionTestHandler | ||
* That way the current test running is marked as failed if any javascript error takes place | ||
* |
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.
Resolves a cosmetic inspector warning.
@@ -65,7 +64,7 @@ public boolean getUseDeveloperSupport() { | |||
|
|||
@Override | |||
protected List<ReactPackage> getPackages() { | |||
return Arrays.<ReactPackage>asList( | |||
return Arrays.asList( |
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.
Resolves a cosmetic inspector warning.
@@ -77,7 +76,6 @@ protected ReactInstanceManager createReactInstanceManager() { | |||
ReactInstanceManagerBuilder builder = ReactInstanceManager.builder() | |||
.setApplication(mApplication) | |||
.setJavaScriptExecutorFactory(getJavaScriptExecutorFactory()) | |||
.setUIImplementationProvider(getUIImplementationProvider()) |
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.
This method no longer exists in RN 0.72.8 and there doesn't seem to be any documentation regarding its replacement.
include("libs:SalesforceAnalytics") | ||
include("libs:SalesforceSDK") | ||
include("libs:SmartStore") | ||
include("libs:MobileSync") | ||
include("libs:SalesforceHybrid") | ||
if (hasNodeModules) { |
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.
This condition and the variables supporting it are no longer needed since the RN artifacts are being resolved from Maven Central.
mavenLocal() | ||
// All of React Native (JS, Objective-C sources, Android binaries) is installed from NPM. | ||
maven("${rootProject.projectDir}/libs/SalesforceReact/node_modules/react-native/android") // For stand-alone MSDK builds. | ||
maven("${rootProject.projectDir}/../../node_modules/react-native/android") // For template app builds. | ||
// Android JSC is installed from NPM. | ||
maven("${rootProject.projectDir}/libs/SalesforceReact/node_modules/jsc-android/dist") // For stand-alone MSDK builds. |
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 do need the JSC local Maven repository still based on on a few tests I tried.
…roid.defaults.buildfeatures.buildconfig Deprecation)
…ault Github Repository References)
5c5ecd9
to
f752487
Compare
Tests results for SmartStoreGenerated by 🚫 Danger |
🎸 Ready For Final Review 🥁
Be aware this pull request is one of four related to the React Native 0.72.7, Gradle 8.2 and Android Gradle Plugin 8.2.0 upgrades.
👉🏻 #2492
👉🏻 forcedotcom/SalesforceMobileSDK-CordovaPlugin#631
👉🏻 forcedotcom/SalesforceMobileSDK-ReactNative#345
👉🏻 forcedotcom/SalesforceMobileSDK-Templates#387
This updates MSDK Android to support the React Native 0.72.8, Gradle 8.1 and Android Gradle Plugin 8.2.0 update in the templates repository. It's a complex change since it dovetailed with the recent updates to Android SDK 34 and other dependencies, plus it has to synchronize with three other repositories.
That said, the updates to MSDK are the smaller and most isolated. So this pull request can begin review while I do final self review and tests on the remaining three.
At the moment, I can build every app and pass all tests here in MSDK Android plus create and run all the template apps for Android and iOS. Overall, the changes are pretty close to merge ready, I'd reckon. I'll take one more self-review of the other three repositories and a final run of all the templates just because of the higher risk for regression with three major third-party dependencies updating.