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

Remove showkase module's dependency on ui-tooling #230

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mxalbert1996
Copy link

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.

Copy link
Collaborator

@vinaygaba vinaygaba left a comment

Choose a reason for hiding this comment

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

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

@vinaygaba
Copy link
Collaborator

@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
Copy link
Author

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
Copy link
Collaborator

@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
Copy link
Author

@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
Copy link
Collaborator

@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
Copy link
Author

mxalbert1996 commented Apr 5, 2022

@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
Copy link
Collaborator

vinaygaba commented Apr 5, 2022

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

@vinaygaba vinaygaba closed this Apr 5, 2022
@mxalbert1996
Copy link
Author

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.

@mxalbert1996
Copy link
Author

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 vinaygaba reopened this Apr 7, 2022
@vinaygaba
Copy link
Collaborator

@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
Copy link
Author

Sure 👍

@mxalbert1996
Copy link
Author

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
Copy link
Collaborator

vinaygaba commented Apr 8, 2022

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

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.

2 participants