-
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(android): add error handling for Kotlin version mismatch #4018
feat(android): add error handling for Kotlin version mismatch #4018
Conversation
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.
Left some comments - we also need remember to update documentation (also we should add info that we require compileSdkVersion
& targetSdkVersion
to be set to 34
android/build.gradle
Outdated
tasks.matching { it.name.startsWith('pre') || it.name == 'compileDebugKotlin' }.all { | ||
dependsOn checkKotlinVersion | ||
} |
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 don't think that we need to do it that we - we can convert checkKotlinVersion
task to function and call this function eg. in buildscript or somewhere else
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.
It's refactored.
Hello, I don't understand which version of kotlin is used in the sample. I tried to look for it and it seems to be 1.7.0, please correct me if I am wrong ? |
@freeboub |
Just for memory: |
I had the issue while checking another bug. To workaround it I just remove : |
@freeboub, if removing the
However, I don't think this dependency is the root cause of the problem. We should investigate further to identify the exact issue. WDYT? |
We should remove it I guess - I have checked template (react-native-create-library) and there is no line like this. You can also look at other libraries |
@KrzysztofMoch you're right. I've checked other libraries like |
We all agree to remove it then :) I wonder if we should put a note in doc for old react native version (0.69 ?) to add it manually in app build.gradle ? |
I think this is good idea - we can put it even in installation section |
We can handle it in |
Ok for me but please check in which react native version kotlin was introduced. If it is before 0.69, no need to add it as it is noticed that 0.69 is the lowest supported version |
@freeboub I checked and noticed that starting from react native version 0.73.0, Kotlin was introduced. So, if the react native version of the app is between 0.69 and 0.72, we can add it. |
Yes, let's go with that ! |
@freeboub @KrzysztofMoch @YangJonghun it has been updated according to the final suggestion. |
android/build.gradle
Outdated
|
||
apply plugin: 'com.android.library' | ||
apply plugin: 'kotlin-android' | ||
|
||
def getReactNativeVersion() { | ||
def packageJsonFile = file("$rootDir/../package.json") |
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.
Package.json is not necessarily in ../ is there any other way to retrieve the version ? I am also wondering if some frameworks can hide react-native in sub package 🤨
android/build.gradle
Outdated
implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version" | ||
} | ||
|
||
if (isReactNativeVersionBetween(reactNativeVersion, minReactNativeVersion, maxReactNativeVersionExclusive)) { |
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.
Here you are blocking build for version < minVersion . I don't think this is really expected...
android/build.gradle
Outdated
def reactNativeVersion = getReactNativeVersion() | ||
|
||
def isReactNativeVersionBetween = { version, minVersion, maxVersionExclusive -> | ||
def versionParts = { v -> v.replaceAll(/[^0-9.]/, '').tokenize('.')*.toInteger() } |
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.
Will it work if user choose a RC version ? In a more general way, is there any gradle package to handle versioning check ?
I just had a review , but in a more general way, I think we shouldn't integrate kotlin on the react native video side, but just let this dependency on the app side. |
@freeboub I couldn't agree more with you. I was fixing this issue last night, and I thought about it. I think it's better to handle it on the app side. As you said earlier, maintaining these changes will be difficult and complex in the future. So, we can update the documentation installation. |
Let's see @KrzysztofMoch's opinion about it, then we can apply it to this PR. |
@freeboub I would like to release |
Summary
Create a custom task to check the Kotlin metadata version.
Motivation
Show clarify error message for Kotlin version mismatch.
And fix: #4009
Changes
Test plan
I changed the Kotlin version in the sample app to
1.7.0
, then ran the app and encountered a new error:Kotlin version mismatch: Project is using Kotlin version '1.7.0' but it must be at least '1.8.0'. Please update the Kotlin version
. After reverting the Kotlin version to1.8.0
or higher, the app worked correctly.