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

breaking: only support androidx #1052

Merged
merged 3 commits into from Apr 15, 2021
Merged

breaking: only support androidx #1052

merged 3 commits into from Apr 15, 2021

Conversation

EinfachHans
Copy link
Contributor

Platforms affected

Android

Motivation and Context

As discussed in #841 , @breautek told me that Version 10.0.0 is planned to only support AndroidX. With this it is possible to use Android Fragments.

Description

  • android.useAndroidX and android.enableJetifier are now set to true by default.
  • The AndroidManifest.xml's Activity Theme changed to the AppCompat one
  • androidx.appcompat:appcompat:1.2.0 added as depencency
  • Usages of android.app.Activity in all Java Files replaced by androidx.appcompat.app.AppCompatActivity
  • Remove testing of normal android-Project

Testing

npm test and manual test with an example project of a plugin of mine, which works with AppCompatActivity. The Preference androidXEnabled is not used anymore.

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@breautek breautek added this to the 10.0.0 milestone Aug 18, 2020
@dpogue
Copy link
Member

dpogue commented Aug 18, 2020

@breautek I think this idea needs further discussion on the dev mailing list. My concern is that using AndroidX by default will break a lot of community plugins, because anything that uses the support library will be incompatible and cause build failures.

Because plugin source files are copied into the app project instead of being distributed as jar/aar files, tools like Jetifier won't upgrade the references to the support libraries.

I'm not saying we shouldn't do this (we probably should), but it needs a discussion about whether we're willing to break a lot of unmaintained plugins for a lot of people.

@EinfachHans
Copy link
Contributor Author

EinfachHans commented Aug 18, 2020

@dpogue we discussed about supporting both at the same time before (as you can see in the linked error) - i mailed the Dev Mailing List about this, but didn't get an answer. Maybe better Solution to make sure Plugins not automatically break?

@dpogue
Copy link
Member

dpogue commented Aug 19, 2020

we discussed about supporting both at the same time before

It's not possible for a project to use both AndroidX and the Android support libraries at the same time.

Currently cordova-android support picking one or the other via a preference in config.xml. However, it is not possible to write a plugin that supports both. You either need two versions of every plugin that uses the libs published to npm, or you have plugins that only work with one configuration option.

There are a lot of apps using 3rd party plugins with dependencies on the Android support libraries, and several of those plugins aren't actively maintained. There is no way to use those plugins in an app that has opted in to AndroidX.

@EinfachHans
Copy link
Contributor Author

@dpogue - Yeah i know, what i mean was, that it (probably in the prepare script) can replace the Usages of android.app.Activity to androidx.appcompat.app.AppCompatActivity and adjust the AndroidManifest.xml if androidXEnabled is set. With a logic like this (or another with the same result) we can use androidx Library and AppCompatActivity only when androidXEnabled is true , so this should break anything?

@faugusztin
Copy link

@HansKrywaa isn't that what the plugin https://github.com/dpa99c/cordova-plugin-androidx-adapter/ does ? To replace old Support Library imports with new AndroidX imports.

@EinfachHans
Copy link
Contributor Author

@faugusztin I don' think it changes the cordova-android Base Classes to use AppCompatActivity instead of android.app.Activity.

@faugusztin
Copy link

@HansKrywaa dpogue's comment was about "unmaintained community plugins". Plugin source codes go into platforms/android/app/src/main, so they are updated to AndroidX imports using the androidx-adapter plugin.

So what could remain as a possible issue are gradle files introduced by plugins, but by the nature of old Android Support Library being abandoned by Google anyway, they will have to be left behind; or people will have to make their own patched versions fixing the Gradle issues, if there are any.

@EinfachHans
Copy link
Contributor Author

Have a Look into this PR. I changed the CordovaActivity to extend AppCompatActivity and not normal Activity anymore. I think this is the Change that will break many Plugins and won't be fixed by the androidx-adapter-plugin 🤔 or am i wrong

@breautek
Copy link
Contributor

Have a Look into this PR. I changed the CordovaActivity to extend AppCompatActivity and not normal Activity anymore. I think this is the Change that will break many Plugins and won't be fixed by the androidx-adapter-plugin or am i wrong

Yah once the framework depends on the androidx support library, this forces everyone, and any plugins they use to also use androidx support libraries rather than the old deprecated support libraries. This is essentially anything inside the android.support namespace.

Therefore, in order to support this in an non-breaking way, we would need to start templating/have two versions of the framework, so that we have a androidx version of the codebase and a non-androidx version of the codebase.

The good news is, if the library is using version 28 of the support libs... then they can easily be upgraded to androidx 1.0.0, as they are binary equivalent. However, like dpogue says, they are a lot of unmaintained plugins that will end up breaking.

To me, while this does pose a problem, but I think holding back poses a larger problem. I don't think holding Cordova back because of unmaintained plugins is a good idea. One of the goals of androidx is to improve android reliability by reducing the need to use reflections in the android code, while still being able to support newer features/APIs and remain backwards compatibility just like before. Moving forward, Google is going to be more strict on reflection usage I think. Holding back will put us in a situation where it may become impossible to support newer features as newer API levels are released.

@EinfachHans
Copy link
Contributor Author

@breautek Yeah think so too. Maintaining both will be to much work , (just) for unmaintained Plugins

@breautek breautek added this to In Progress in Release Plan - 10.1.0 via automation Mar 27, 2021
@erisu erisu requested a review from knight9999 March 31, 2021 07:08
Copy link

@knight9999 knight9999 left a comment

Choose a reason for hiding this comment

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

I have created & built a sample project with this android platform.

But I met Cannot access androidx.appcompat.app.AppCompatActivity error.

In order to resolve this error, i should add dependencies

    implementation 'androidx.appcompat:appcompat:1.2.0'

in platforms/android/CordovaLib/build.gradle .
in bin/templates/project/app/build.gradle.

I think it is necessary to improve the bin/templates/project/app/build.gradle according to.

Other than this error, it works fine.

@brodybits
Copy link
Contributor

I don't think holding Cordova back because of unmaintained plugins is a good idea.

I think another benefit of moving forward could be to encourage user community members to fork and publish modernized, supported versions of some unmaintained plugins. Beauty of open source!

Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

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

This PR has conflicts that needs to be resolved and fails to build.

See @knight9999 comments.

Release Plan - 10.1.0 automation moved this from In Progress to Review in progress Apr 14, 2021
@EinfachHans
Copy link
Contributor Author

@erisu @knight9999 added 😊

@erisu
Copy link
Member

erisu commented Apr 14, 2021

@EinfachHans Thank you for the fix.

Can you also rebase the branch and resolve the conflicted files?

I believe three of the files you deleted so it should be easy to fix, but the Java file I have not looked at so it might be easy to fix too.

Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@knight9999 knight9999 left a comment

Choose a reason for hiding this comment

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

It looks good.

Copy link
Member

@NiklasMerz NiklasMerz left a comment

Choose a reason for hiding this comment

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

CODE LGTM

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

Since the theme was changed, this test should be changed too.
https://github.com/apache/cordova-android/blob/master/spec/unit/AndroidManifest.spec.js#L43

The theme is not really being checked, so not changing it won't make tests fail, but would be good if it could match.

I won't block the merge if it doesn't get changed.

@erisu erisu merged commit 2a84d7c into apache:master Apr 15, 2021
Release Plan - 10.1.0 automation moved this from Reviewer approved to Done Apr 15, 2021
wmathurin added a commit to wmathurin/SalesforceMobileSDK-CordovaPlugin that referenced this pull request Sep 21, 2021
Lining up with Cordova's template changes (see apache/cordova-android#1052)
wedgberto pushed a commit to wedgberto/cordova-android that referenced this pull request May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

9 participants