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

Use AndroidX #665

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Use AndroidX #665

wants to merge 20 commits into from

Conversation

cristan
Copy link

@cristan cristan commented Aug 21, 2019

Fixes #662

This enables people turning of jetifier when using Stetho.

This is the last version which doesn't complain about 'variant.getJavaCompile()' being obsolete. This should be addressed when upgrading the gradle dependencies further.
The new AndroidX dependencies have a minSdkVersion of 14, so Stetho's minSdkVersion has to be updated as well.
I've only updated to the latest minor version of that same release. The latest proper version of that dependency probably works just as well, but this PR is focused on AndroidX.
It has been documented as optional, but I doubt that's actually the case. The problem is that I can't define POM_OPTIONAL_DEPS with androidx.fragment:fragment as value because it is already defined.

implementation 'com.android.support:appcompat-v7:23.0.1' // optional
implementation 'androidx.appcompat:appcompat:1.0.2' // optional

Choose a reason for hiding this comment

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

If it's optional it should be compileOnly.
AppCompat is actually used nowhere in the code. Only Core and Fragment.

Choose a reason for hiding this comment

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

compileOnly is probably a no go? https://issuetracker.google.com/issues/109894265

Choose a reason for hiding this comment

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

That should only be an issue if you're trying to work with Android resources from the library, which stetho doesn't, at least as far as I can see. As long as the error doesn't pop up during compilation we're golden.

Choose a reason for hiding this comment

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

the resources still contribute to the R.class file generation which "could" lead to issues for consumers maybe at build time maybe at runtime maybe in a lint run?

I agree it is probably not anything to worry about, but I am more making the argument that if we know it isn't correct it would be best to just avoid it and star the aosp issue to hopefully move it up in the priority list.

Choose a reason for hiding this comment

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

Oh. That. Yeah, that could be an issue for some unfortunate souls.

Defensively, call it a breaking change and raise major version. If in exchange I get rid of hard dependency of AppCompat, I'd consider it an acceptable price.

Choose a reason for hiding this comment

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

Can be, we need AndroidX Core, which until now was pulled in automatically as a dependency of Fragment. This should work:

implementation 'androidx.core:core:1.1.0'
compileOnly 'androidx.fragment:fragment:1.1.0'

I get error: cannot find symbol everywhere

The "everywhere" part wasn't helpful at all. Knowing which imports didn't resolve (and maybe seeing that all were from androidx.core package) would be.

Stop assuming failure, this is going to work ;)

Copy link
Author

@cristan cristan Oct 15, 2019

Choose a reason for hiding this comment

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

Do we actually need AndroidX core? The only places with import androidx.core are AccessibilityUtil and AccessibilityNodeInfoWrapper. AccessibilityUtil is only used in AccessibilityNodeInfoWrapper is only used from ViewDescriptor where we check whether we can access androidx.core.view.accessibility.AccessibilityNodeInfoCompat.class.getClass(). So, as far as I can see, core is just as optional as the Fragment dependency. Is there anywhere else we use core that I missed?

The "everywhere" part wasn't helpful at all

Good point: it actually isn't everywhere. It's everywhere where we use any transient dependencies like core.

Adding all the transient dependencies works:

compileOnly 'androidx.core:core:1.1.0@aar'
compileOnly 'androidx.lifecycle:lifecycle-common:2.1.0'
compileOnly 'androidx.lifecycle:lifecycle-viewmodel:2.1.0@aar'
compileOnly 'androidx.savedstate:savedstate:1.0.0@aar'
compileOnly 'androidx.activity:activity:1.0.0@aar'
compileOnly 'androidx.fragment:fragment:1.1.0@aar'

-edit: core was lost because of a formatting issue, it's now back again.

Not using @aar also gets rid of the errors:
compileOnly 'androidx.fragment:fragment:1.1.0

What shall we use?

Choose a reason for hiding this comment

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

I don't think we need all the dependencies of Fragment. We only need those that we need to compile, whose API we use in our code. Fragment checks out. Core, as you mentioned, too.

If the accessibility stuff is optional, compileOnly is fine (with its own ReflectionUtil check). Otherwise implementation is the way to go. So this is it?

compileOnly 'androidx.core:core:1.0.0@aar'
compileOnly 'androidx.fragment:fragment:1.0.0@aar'

Mind the 1.0.0 as we don't want to accidentally access something that's not in v1. That way the consumer is free to choose anything from [1.0.0, 2.0.0).

Plus implementation annotations, of course.

Awesome!


I'd add proguard rules to ignore missing Fragment and Core.

-dontwarn androidx.fragment.**
-dontnote androidx.fragment.**
-dontwarn androidx.core.**
-dontnote androidx.core.**

Otherwise consumers using Proguard would get fails, if they don't use Fragment or Core, about missing classes.

It would be better to name concrete classes or entry points that are referenced in code, but this should be fine.

Or the other way around, specify the accessing classes in this project, that should work. So many possibilities!


A potential issue is that the whole module is free to reference classes from Core and Fragment but it's no longer guaranteed to be in runtime classpath. These "optional" accessors should be in separate modules, which won't leak their compileOnly dependency to the main Stetho library.

That touches publishing, which may be a problem.

Copy link
Author

@cristan cristan Oct 15, 2019

Choose a reason for hiding this comment

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

Just using Core and Fragment as @aar dependencies doesn't work:

compileOnly 'androidx.fragment:fragment:1.0.0@aar'

You'll get the following errors:

/Users/Cristan/repos/stetho/stetho/src/main/java/com/facebook/stetho/common/android/FragmentCompatSupportLib.java:72: error: cannot access LifecycleOwner
      return fragment.getFragmentManager();
                     ^
  class file for androidx.lifecycle.LifecycleOwner not found
/Users/Cristan/repos/stetho/stetho/src/main/java/com/facebook/stetho/common/android/FragmentCompatSupportLib.java:77: error: cannot access ViewModelStoreOwner
      return fragment.getResources();

If you want AndroidX Fragment 1.0.0 as an AAR, you'll need to define the following to solve this:

compileOnly 'androidx.lifecycle:lifecycle-common:2.0.0'
compileOnly 'androidx.lifecycle:lifecycle-viewmodel:2.0.0@aar'
compileOnly 'androidx.core:core:1.0.0@aar'
compileOnly 'androidx.fragment:fragment:1.0.0@aar'

Anyway: making the optional dependencies actually optional hasn't got anything to do with AndroidX: your master branch has the same issues. I've therefore just updated the comment.

I suggest to merge this PR and create a separate issue to make the dependency actually optional (or go the non-optional route and remove the realtime checks).

Copy link
Author

Choose a reason for hiding this comment

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

I've just removed the last code which still didn't assume a minSdkLevel of 14. I've removed the documentation about ActivityTracker.add and remove: the AutomaticTracker now works for everyone, so I don't think we should even mention that you can fiddle with the added / removed activities. You could even argue for making add and remove private methods.

@cristan cristan force-pushed the master branch 4 times, most recently from f2e1778 to c534fdf Compare October 15, 2019 22:17
Cristan Meijer added 3 commits October 16, 2019 00:33
We purposefully depend on version 1.0.0 because we don't want to accidentally access something that's not in v1. This way, the consumer is free to choose anything from [1.0.0, 2.0.0).
@consp1racy
Copy link

Compile SDK version should be raised to 29 if it's not already. That may result in even more nullability annotations.

@cristan
Copy link
Author

cristan commented Oct 16, 2019

Compile SDK version should be raised to 29 if it's not already. That may result in even more nullability annotations.

  • compileSdkVersion and targetSdkVersion are now at 29
  • Missing non-null annotations are added
  • Redundant casts at findViewById removed (which were no longer needed after targeting 26)

@consp1racy
Copy link

consp1racy commented Oct 18, 2019

I didn't find the compileOnly dependencies in changes. As you mentioned this should be it:

compileOnly 'androidx.lifecycle:lifecycle-common:2.0.0'
compileOnly 'androidx.lifecycle:lifecycle-viewmodel:2.0.0@aar'
compileOnly 'androidx.core:core:1.0.0@aar'
compileOnly 'androidx.fragment:fragment:1.0.0@aar'

I found this instead:

implementation 'androidx.fragment:fragment:1.0.0@aar'

The two approaches differ:

  1. Keep implementation and force consumers manually excluding the dependency on fragments and core AndroidX. The consumers shouldn't need to know that Stetho can somehow use fragments.
  2. Go with compileOnly and... that's it. The consumer will add fragment dependency if they wish to use fragments. And only then condiditional code in Stetho will kick in.

@cristan
Copy link
Author

cristan commented Oct 18, 2019

I didn't find the compileOnly dependencies in changes. As you mentioned this should be it:

compileOnly 'androidx.lifecycle:lifecycle-common:2.0.0'
compileOnly 'androidx.lifecycle:lifecycle-viewmodel:2.0.0@aar'
compileOnly 'androidx.core:core:1.0.0@aar'
compileOnly 'androidx.fragment:fragment:1.0.0@aar'

I found this instead:

implementation 'androidx.fragment:fragment:1.0.0@aar'

@consp1racy That's on purpose: I don't think this should be part of this PR anymore. That's because making the optional dependencies actually optional hasn't got anything to do with AndroidX. Therefore, let's just merge this PR and create a separate issue to make the dependency actually optional (or go the non-optional route and remove the realtime checks).

@pandelisgreen13
Copy link

There is any update for next release with AndroidX?

@erikghonyan
Copy link

@cristan Thanks for getting the ball rolling! Are you still looking into this?

@cristan
Copy link
Author

cristan commented Mar 16, 2020

@cristan Thanks for getting the ball rolling! Are you still looking into this?

I don't think there's anything for me to do: all AndroidX related suggestions are implemented.

Unfortunately, this project seems to be dead: only 1 commit has been done on this project since March 2019 (see #677). I'm now using another solution, I suggest y'all do the same.

@lmj0011
Copy link

lmj0011 commented Jul 30, 2020

I'm now using another solution, I suggest y'all do the same.

I assume that solution is Flipper?

@cristan
Copy link
Author

cristan commented Jul 30, 2020

It isn't. I've tried Flipper but I ran into this issue. That was in 2018 and despite saying this is something they'd like to improve, they never did. I now use chucker.

@scottyab
Copy link

Thanks for the work here @cristan.

mdzyuba added a commit that referenced this pull request Mar 9, 2021
Partially merge pull request #665.

Update to 1.6.0-SNAPSHOT
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Androidx
8 participants