-
Notifications
You must be signed in to change notification settings - Fork 108
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
Generate code to have an API that provides module level metadata of Showkase elements #392
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@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.
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() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
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