Skip to content
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

Merged
merged 13 commits into from
Jul 14, 2019
Merged

AndroidX Support #112

merged 13 commits into from
Jul 14, 2019

Conversation

mrbrentkelly
Copy link
Contributor

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.

@@ -0,0 +1,2 @@
android.enableJetifier=true

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed 👍

@@ -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

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.

Copy link
Contributor Author

@mrbrentkelly mrbrentkelly Jul 10, 2019

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)

Copy link

@se-bastiaan se-bastiaan Jul 10, 2019

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.

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

Copy link
Contributor Author

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

@cooperka
Copy link
Owner

cooperka commented Jul 8, 2019

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).

@bhaskarGyan
Copy link

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()

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.

Copy link
Contributor Author

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 👍

@bhaskarGyan
Copy link

Hi @se-bastiaan , @cooperka Should I open a new PR with above changes?
Looks like @mrbrentkelly is busy.

@cooperka cooperka mentioned this pull request Jul 13, 2019
@cooperka
Copy link
Owner

Fantastic work here, thank you all 🎉

Tested and verified on Android and iOS. Will bump major version to 2.0. Much appreciated.

@cooperka cooperka merged commit 4ca83a6 into cooperka:master Jul 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants