Skip to content

Conversation

@vinaygaba
Copy link
Collaborator

@vinaygaba vinaygaba commented Jun 10, 2024

This PR adds a new public API that generates metadata about the Showkase elements on a per module basis. This essentially results in a new API being exposes which gives you access to a Showkase.getModuleMetadata() method in every module where Showkase is configured to run.

The reason this is useful is so that this API can be leveraged to define custom tooling and behavior, such as generating a Paparazzi test in each module separately (as opposed to doing it in just the root module with all the previews aggregated). This allows the tests to be better parallelized and even avoided if the module did not have changes.

With this change, a new API is available for you to get access to all the Showkase elements in a given module. Here's what it looks like at the call site -

Showkase.getModuleMetadata()

Addresses: #299 and #379

@elihart @airbnb/showkase-maintainers

Copy link
Contributor

@elihart elihart left a comment

Choose a reason for hiding this comment

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

Thanks for adding this @vinaygaba ! wasn't sure if you wanted review yet since its still marked as draft and CI is failing, but I took a look.

I wasn't sure if there should be codegen tests showing the api difference, is that still coming?

Does anything need to be added to the test codegen to take advantage of this, or will per module tests just work once this is set up?

.propertyPackage

internal fun Collection<ShowkaseMetadata>.getNormalizedPackageName() = this
.first()
Copy link
Contributor

Choose a reason for hiding this comment

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

is it safe to assume this is non null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do a check before calling this but maybe I do this at the util level so that it's future proof.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it's better to be safe at the util level to not make assuumptions about where its called

@vinaygaba
Copy link
Collaborator Author

@elihart I'll do the test update once we agree on the API direction. Those just need to be regenerated and I'm happy to add them upfront if that helps wrap your head around the changes.

Does anything need to be added to the test codegen to take advantage of this, or will per module tests just work once this is set up?

This is meant to be done in a follow up PR. This change is particular can happen in isolation so figured it makes doing reviews easier.

moduleShowkaseBrowserProperties: ShowkaseBrowserProperties,
) {
if (moduleShowkaseBrowserProperties.isEmpty()) return
val packageName = moduleShowkaseBrowserProperties.getPackageName()
Copy link
Collaborator Author

@vinaygaba vinaygaba Jun 10, 2024

Choose a reason for hiding this comment

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

@elihart One of the challenges is which package to generate this API in. In the existing metadata APIs, we generate them in the package of the ~@ShowkaseRoot` module. As this file is generated in every module, I'm currently generating it in the package of the first element from this list. I don't quite like that. A fixed package isn't an option either since the API is identical in each module. Do you have any alternate suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

One of the challenges is which module to generate this API in

Do you mean, "which package"?

It's hard for me to reason through what the new API looks like without examples in the PR summary or in the code - could you add something to show what the current output looks like?

What's your current strategy for how the test source codegen would know where to find the components for that module?

I think you may need to use an annotation to indicate the package. This could either be a new annotation (like @ShowkaseModule) or possibly you could reuse @ShowkaseRoot with the new expectation that it can be used in multiple modules.

Another option that comes to mind is that you could find the parent package common to all components in the module being processed. For example; com.example.a.b.foo and com.example.a.c.foo are processed and you identify com.example.a as the common package to generate the code in. The downside to this is it makes the output location a bit unpredictable, and prone to changing unexpectedly, so I think you probably need to use a fixed annotation.

For Airbnb, we could avoid manually adding that annotation, and only add it at CI time in each ui module before running code gen (and a similar process to also add the necessary annotation in the test sources for the test codegen)

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.

3 participants