firebase / firebase-android-sdk

Firebase Android SDK
https://firebase.google.com
Apache License 2.0
2.27k stars 580 forks source link

161731384: Move to Picasso that has no support deps after it is released #1736

Closed ubuntudroid closed 3 years ago

ubuntudroid commented 4 years ago

What feature would you like to see?

Right now you need to enable jetifier if you pull in the firebase-inappmessaging-display dependency as reported in #535 . This is due to a dependency on picasso which pulls in com.android.support:exifinterface:

com.android.support:exifinterface:27.1.0
\--- com.squareup.picasso:picasso:2.71828
     +--- debugCompileClasspath (requested com.squareup.picasso:picasso:{strictly 2.71828})
     \--- com.google.firebase:firebase-inappmessaging-display:19.0.7
          +--- debugCompileClasspath (requested com.google.firebase:firebase-inappmessaging-display)
          \--- com.google.firebase:firebase-bom:25.5.0
               \--- debugCompileClasspath

However, enabling jetifier, while undoubtedly handy in cases like these, unnecessarily slows down builds.

How would you use it?

I'd disable jetifier and enjoy faster builds. 🚀

ashwinraghav commented 4 years ago

Thanks for raising @ubuntudroid This seems like a valid concern given. We've made the effort to move away our direct dependencies on support over to jetpack libraries and this makes that effort futile.

I bet there are image processing libraries that don't have similar problems. The scope of the fix is really limited to migrating the dependency on Picasso out to some other library. The migration is tightly scoped to this one file if you are inclined to send a fix our way 🥇

https://github.com/firebase/firebase-android-sdk/blob/d9ed980c39c58ddf55ab945baa120c2e0a9fa8cb/firebase-inappmessaging-display/src/main/java/com/google/firebase/inappmessaging/display/internal/FiamImageLoader.java#L31

ubuntudroid commented 4 years ago

@ashwinraghav From what I see there are two options:

  1. Stay with Picasso and wait for 3.0.0 to be released (no ETA from what I know though). Their SNAPSHOT version has already been migrated to androidx - however I'm pretty sure, using snapshot versions is out of the question for a widely used library as the Firebase SDK.
  2. Migrate to some other library (Glide anyone?) as you suggested.
ashwinraghav commented 4 years ago

Thanks for pointing that out. I'll leave this open until we have a version of Picasso we can migrate to. It's probably not worth the effort to move libraries given that Picasso is expected to be on Jetpack at some point .

thatfiredev commented 4 years ago

I think we should also consider FR #1025 which might solve this problem.

Update: Oops, I only noticed now that it has been closed in favor of improving the current image loader. My bad

lordcodes commented 4 years ago

@ashwinraghav Can we consider a different solution to "wait for Picasso 3.0". There is no ETA on this and it has been in-progress for quite a while. Also, it seems wasteful for any app that is using a different Image Loader such as Coil to include Picasso. I think Firebase In-app Messaging may be the only dependency requiring the use of Jettifier for many projects.

Is it possible to use Firebase In-app Messaging without it forcing the use of Picasso, such as by implementing the "display" component yourself? Or would it be possible to implement downloading an image to display in the in-app message within the library?

ashwinraghav commented 4 years ago

@lordcodes Thanks for the follow up. To your point, here's something we can do:

  1. Have the Image Loader implement an interface and externalize this implementation as a separate library (say) com.google.firebase:fiam-image-loader. Fiam will ship with a dependency on this externalized library by default, since it would be a (severe) breaking change otherwise.

  2. Customers who want to use an alternate image library can exclude this library

    implementation ("com.google.firebase:firebase-inappmessaging:....") {
     exclude "com.google.firebase:fiam-image-loader"
    }

    And provide their own implementation of the aforementioned interface, and add a manifest entry for Firebase to discover the implementation..

This becomes the case since providing a default implementation that requires no programmatic interaction is imperative to avoid this be a breaking change for most developers.

lordcodes commented 4 years ago

@ashwinraghav What you have described is almost exactly what I would like to see.

Would be very eager to see this!

ashwinraghav commented 4 years ago

If you are inclined to contribute, pls send us a PR and I'm happy to review your change.

rashadsookram commented 4 years ago

It turns out there isn't a need to wait for Picasso 3.0. Picasso 2.8 was recently released, and it migrated over to androidx: https://github.com/square/picasso/pull/2176.

rohithThammaiah commented 4 years ago

Any update on Migration to 2.8?

lordcodes commented 4 years ago

Rather than waiting you can use Gradle to force Picasso 2.8 to be used.

In Gradle Kotlin DSL:

implementation("com.squareup.picasso:picasso") {
    version {
        strictly("2.8")
    }
    because("Firebase In-App Messaging Display includes a Picasso version that doesn't use AndroidX")
}
blundell commented 3 years ago

Fixed in: https://github.com/firebase/firebase-android-sdk/pull/2408

mikehardy commented 3 years ago

Moved from Picasso to Glide - merged as #2068 on branch master with no release tags yet, I suppose we're just waiting on release?

c.f. https://github.com/firebase/firebase-android-sdk/commit/bc850aa4cb8298b30598bd30da661b591e784d4b

mikehardy commented 3 years ago

Apologies just saw BoM 28.4.0 was released with this in it! :trophy: https://firebase.google.com/support/release-notes/android#inappmessaging_v20-1-0

eldhosembabu commented 3 years ago

Apologies just saw BoM 28.4.0 was released with this in it! 🏆 https://firebase.google.com/support/release-notes/android#inappmessaging_v20-1-0

Closing this thread since the glide migration is released. Feel free to reopen in case of any issues.