airbnb / Showkase

🔦 Showkase is an annotation-processor based Android library that helps you organize, discover, search and visualize Jetpack Compose UI elements
https://medium.com/airbnb-engineering/introducing-showkase-a-library-to-organize-discover-and-visualize-your-jetpack-compose-elements-d5c34ef01095
Apache License 2.0
2.11k stars 107 forks source link

Handle private @Preview annotations gracefully #250

Closed micHar closed 1 year ago

micHar commented 2 years ago

When trying to build a project with private Previews, Showkase complains:

The methods annotated with Preview can't be private as Showkase won't be able to access them otherwise

Does it have to be an error? Not all previews are Showkase-worthy, some of them can be used just in ide. Maybe it could be a warning or there could be a gradle flag for this? Wdyt? This should be relatively easy, I could contribute if you decide that it's a good idea to add it.

micHar commented 2 years ago

Tbh, I would go as far as to include an option to opt out of using @Preview annotations altogether. If I can achieve all the same things with a dedicated showkase annotation, it makes the intent more explicit and is more future-proof as custom annotations will inevitably have more parameters than @Preview.

vinaygaba commented 2 years ago

@micHar Actually, there is an option that's meant to do exactly this. Use the skip property in @ShowkaseComposable. Essentially, the preview will look like this.

@ShowkaseComposable(skip=true)
@Preview
private fun MyPrivateComposable() {

}
micHar commented 2 years ago

Hmm, I'm not a big fan of this solution. It's like the library hijacks the more general annotation for its specific purpose. Would be more elegant to at least ignore the private preview and not crash the build when lib can be graceful about it.

vinaygaba commented 2 years ago

@micHar The reason this was done is because silently ignoring would be a lot worse since the expectation for most new users would be that this just works out of the box. Maybe I can instead make this a warning so that at least the user is notified in some way.

micHar commented 2 years ago

Yes, seems like a better idea to warn than to crash. Generally I would agree with you when it comes to the fail fast principle, but here it feels like showkase should be an addition and not hijack the original process. Alternatively maybe a compiler flag or sth to disable this behavior so that we don't have to use showkase annotations on composables which are not meant for showkase at all.

iamutkarshtiwari commented 2 years ago

@vinaygaba I'd also be nice to throw the lines in the error log at which the @Preview annotated methods are private. This was we can quickly navigate to that specific erroneous method and fix the build issue.

I was trying to integrate showkase in my project which has around 100s of @Preview annotated methods and showkase was throwing this aforementioned error. It took me a lot of time to find the ones that were actually failing the build.

vinaygaba commented 2 years ago

@iamutkarshtiwari Yeah I'm surprised I had not added that already. I plan to fix this issue this week and I will improve the error logging as well so that you know the source of the error.

iamutkarshtiwari commented 1 year ago

@vinaygaba Thank you for your response!

One more suggestion. I have a library that has several preview methods in it. Preview methods allows devs to see their changes realtime when making changes to the code and in order to be able to use these previews in showkase, we have to make them public. There is one downside though, because of this, these previews would also leak also in the public API on the consumer side. Would it be possible to make showkase work with internal previews instead of having them public?

vinaygaba commented 1 year ago

Fixed the original issue that was flagged. Will add documentation when I'm ready for the next release, otherwise it will cause confusion.

@iamutkarshtiwari I do have some ideas on how you can avoid the previews from the public API. Maybe I should add a section in the README section. If I did allow internal methods, every module will only have its own set of components. It won't be able to aggregate all components across your app, which is probably want you want from a discovery stand point (which is why I created this library).

YalcinDelioglu-Ahold commented 1 year ago

Thanks for resolving this. When is it going to be released, @vinaygaba ?

iamutkarshtiwari commented 1 year ago

If I did allow internal methods, every module will only have its own set of components. It won't be able to aggregate all components across your app, which is probably want you want from a discovery stand point (which is why I created this library).

@vinaygaba Sorry I didn't quite understand. Do you mean to add a feature to allow internal preview methods to be aggregated or is there already a functionality that I am missing?

vinaygaba commented 1 year ago

For future reference, I want to summarize how this issue is now being handled by Showkase -

In the beta15 release, I added a flag to allow you to skip private previews altogether - https://github.com/airbnb/Showkase/releases/tag/1.0.0-beta15

In the beta 17 release, a big refactor has now opened up the possibility of keeping your previews internal instead of forcing them to be public - https://github.com/airbnb/Showkase/releases/tag/1.0.0-beta17

You can choose either of these options depending on what makes sense for your use case.