Remove showkase module's dependency on ui-tooling

[mxalbert1996]

Summary

androidx.compose.ui:ui-tooling doesn't seem to be used anyway in the main module so remove it to avoid pulling in unneeded dependencies for consumers.

Background

We are using ui-tooling-preview as release dependency and ui-tooling as debug dependency as suggested here to avoid including PreviewActivity and other stuff unneeded in release builds. However Showkase pulls in ui-tooling even for release builds.

[vinaygaba]

I just realized I hadn't added documentation on how to setup Showkase more efficiently. I'll add that documentation soon!

[vinaygaba]

@mxalbert1996 Actually thinking a bit more, I think we don't need this change. Instead, use the following setup with Showkase (I'll also add this to the README) -

debugImplementation "com.airbnb.android:showkase:1.0.0-beta12"
implementation "com.airbnb.android:showkase-annotation:1.0.0-beta12"
kaptDebug "com.airbnb.android:showkase-processor:1.0.0-beta12"

This will ensure that you don't face the issue that you described in your ticket. Give it a shot and let me know if it fixes the issue for you and I will then go ahead and also update the README

[mxalbert1996]

Actaully I've thought of that but the problem is that in order to use Showkase.getBrowserIntent(context) we need com.airbnb.android:showkase. If we go this way, we have to put that in debug source set and create a placeholder in release source set, which is annoying.

[vinaygaba]

@mxalbert1996 Yes that's actually how we are using it. This is actually a really full proof way to ensure that your previews are not being referenced by any piece of code in non-release builds. This will also guarantee that your preview functions have no possibility of being retained in release builds (which is what you'd ideally want)

[mxalbert1996]

@vinaygaba
Do you mean you are putting all the preview functions in debug source set? That's a lot of setup and prevents preview of private functions so we definitely won't go this way.
Our preview functions are easy to recognize as they all end with "Preview" so we won't call them in non-preview code, and R8 will ensure they are all stripped out.
It's just that it's so easy to fix the problem from Showkase side and you are not using ui-tooling in the first place so why not?

[vinaygaba]

@mxalbert1996 no I meant purely having the Showkase root class that's annotated with @ShowkaseRoot along with the activity that references Showkase.getBrowserIntent(context) in the debug source set. The preview functions themselves continue to be anywhere they want to be.

Our preview functions are easy to recognize as they all end with "Preview" so we won't call them in non-preview code, and R8 will ensure they are all stripped out.

Not if something is referencing these preview functions in the release builds. It's possible that if you use Showkase for release builds, its autogen code will hold a reference to these preview functions. Maybe R8 is still intelligent enough to strip them off but by simply having these 2 files in the debug source set, you guarantee that the preview functions will never make its way into the release build (at least not due to Showkase)

It's just that it's so easy to fix the problem from Showkase side and you are not using ui-tooling in the first place so why not?

Showkase does have first class support for @Preview annotation i.e you don't need to annotate a function with @ShowkaseComposable if you are already using @Preview. So it does require this dependency. Although, I think one more improvement should be to rely on deps.compose. toolingPreview insead of deps.compose. tooling as Showkase just needs the annotation and not the entire tooling dependency.

[mxalbert1996]

@vinaygaba

Maybe R8 is still intelligent enough to strip them off

Yes, definitely intelligent enough. That's one of the reasons why R8 exists.

Showkase does have first class support for @Preview annotation i.e you don't need to annotate a function with @ShowkaseComposable if you are already using @Preview. So it does require this dependency.

IMHO we don't call this "require". If Showkase requires ui-tooling, it won't work without ui-tooling, which is not true. And same for ui-tooling-preview.
The fact that Showkase is depending on a library which it isn't using in its code and doesn't require at runtime itself seems strange.

[vinaygaba]

@mxalbert1996 end user doesn't require it but Showkase needs it internally to add support for @Preview annotations. Anyway I've suggested what you should be using and what we use internally. In addition, I do have one minor update I can make. Going to close out your PR and this conversation.

[mxalbert1996]

OK, it's your library after all but it would be kind of you if you could tell me where you are using ui-tooling as I've searched through the code before creating this PR and there are no occurrences of androidx.compose.ui.tooling in showkase and showkase-annotation modules.

[vinaygaba]

@mxalbert1996 It's here - https://github.com/airbnb/Showkase/blob/master/showkase-processor/src/main/java/com/airbnb/android/showkase/processor/ShowkaseProcessor.kt#L412

[mxalbert1996]

But that's showkase-processor, isn't it? Unlike showkase and showkase-annotation, showkase-processor isn't a implementation dependency (it's a ksp/kapt one instead) and will not be in the runtime classpath so its dependencies don't matter.

[vinaygaba]

@mxalbert1996 I have it mixed up and I stand corrected 😱 The dependency I was suggesting (ui-tooling-preview) is needed in the showkase-processor (and is currently missing there) and can be removed from showkase (like you are doing in this PR). Do you want to go ahead and add that dependency in the processor module as that has been something that keeps slipping my mind.

[mxalbert1996]

Sure 👍

[mxalbert1996]

It seems that showkase-processor is a java module thus can't depend on any Android (aar) libraries including ui-tooling-preview.
However the current way of specifying the fully-qualified name of Preview class should be OK without the dependency.

[vinaygaba]

@mxalbert1996 This actually rings a bell. I think that's the reason the dependency didn't live in the processor already.

However the current way of specifying the fully-qualified name of Preview class should be OK without the dependency.

This is not what I experienced. I actually had a todo in my internal usage of Showkase

// Remove this once showkase-processor adds this dependency in the next release
implementation composeDeps.uiToolingPreview

This was needed because Showkase processor wasn't able to find the @Preview annotation using the fully qualified name without the dependency being available in some fashion.