-
Notifications
You must be signed in to change notification settings - Fork 151
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
AndroidX Support #112
AndroidX Support #112
Conversation
android/gradle.properties
Outdated
@@ -0,0 +1,2 @@ | |||
android.enableJetifier=true |
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 not necessary since there are no other dependencies that this library depends on.
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.
Removed 👍
example/android/gradle.properties
Outdated
@@ -16,3 +16,5 @@ | |||
# This option should only be used with decoupled projects. More details, visit | |||
# http://www.gradle.org/docs/current/userguide/multi_project_builds.html#sec:decoupled_projects | |||
# org.gradle.parallel=true | |||
android.enableJetifier=true |
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 not necessary since there are no other dependencies that this library depends on.
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.
@se-bastiaan if I remove this line then the sample app fails to build. Maybe because it's running an older version of React Native (0.56.0), which will have Android dependencies that need run through the jetifier? (The fix in that case would be to upgrade react-native-snackbar to RN 0.60, or run with jetifier enabled and upgrade in a subsequent 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.
I believe that would be the reason indeed. I'm not sure it is wise to give an example with <0.60 if the update to the package itself will possibly break applications that have not updated to RN 0.60. At least if they do not use jetifier themselves. I'm seeing a lot of different approaches here, I'm not even sure what the proposed option by the react-native maintainers is exactly. In the case of this package I think it is a valid approach to drop support for pre-0.60 and go to version 2.0.0, since it heavily depends on the support library/material library to provide the UI. In that case the example would also need to be 0.60.
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.
Okay so it seems that a special jetifier tool for react-native is the recommended path to updating: https://github.com/mikehardy/jetifier
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.
Agreed, I think we should go to version 2.0.0 👍.
I have went ahead and updated the example app to React Native 0.60.3. @se-bastiaan, could you give this PR another code review 🙏?
cc @cooperka
Much appreciated @mrbrentkelly, I'll merge this once v0.60 is released (feel free to remind me when that happens as I'm not highly active with RN these days). |
Hi @cooperka , v0.60 is released, Can you please merge |
@@ -27,5 +27,6 @@ allprojects { | |||
// All of React Native (JS, Obj-C sources, Android binaries) is installed from npm | |||
url "$rootDir/../node_modules/react-native/android" | |||
} | |||
google() |
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.
Lines 23-25 already contain this maven repository. Those lines can be replaced by these.
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.
Updated to match a fresh React Native project 👍
Hi @se-bastiaan , @cooperka Should I open a new PR with above changes? |
Example app 0.60 upgrade
Fantastic work here, thank you all 🎉 Tested and verified on Android and iOS. Will bump major version to 2.0. Much appreciated. |
This PR updates the android project (and example android project) to support AndroidX.
React Native will support AndroidX once 0.60 is released, so it's important that third party libraries also make the switch.
https://github.com/facebook/react-native/releases/tag/v0.60.0-rc.1
facebook/react-native#23112
Note: I would advise NOT merging this until RN0.60 has been released.