-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
Support KMP & Jetbrains compose multiplatform #110
Support KMP & Jetbrains compose multiplatform #110
Conversation
|
UPDATE: I managed to get samples working and test the plugin locally. I found a small issue which is about registering gradle tasks for jvm & multiplatform at the right time when their target info is made available(we're doing it correctly for android modules using |
Found no issues so far after fixing those timing bugs. I'm also thinking about pushing samples(multiplatform, android & desktop) to the repo to make it easier to test any changes right now or in the future & provide a rapid feedback loop but I'll probably do it in another PR to make changes easier to review and to keep this PR focused on one single topic, WDYT? |
@mr3y-the-programmer That's great. I've not yet looked into the PR changes. Let me get back to you after taking a look. I won't be able to spend much time here, so please expect a delay |
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.
@mr3y-the-programmer Did first round of review. Have some comments, can you please look into them? Plugin changes LGTM 👍🏻 Nice work.
...otlin/dev/shreyaspatil/composeCompilerMetricsGenerator/core/ComposeMetricsContentProvider.kt
Outdated
Show resolved
Hide resolved
...nMain/kotlin/dev/shreyaspatil/composeCompilerMetricsGenerator/core/mapper/ConditionMapper.kt
Outdated
Show resolved
Hide resolved
Did the required modifications based on the recent comments, take your time and see if you've any other questions |
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.
This changeset actually looks good to me. Nice work here and I appreciate your contribution 😃. I've few more comments here.
val composeMultiplatform = | ||
runCatching { | ||
target.extensions.getByType(ComposeExtension::class.java) | ||
}.getOrNull() |
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'll get back here with something if there's anything else by which we can write this in better way. I've did something like this in my another plugin: https://github.com/PatilShreyas/bytemask/blob/v1.0.0-alpha01/gradle-plugin/src/main/java/dev/shreyaspatil/bytemask/plugin/BytemaskPlugin.kt#L40-L52
Can we do similar here?
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.
Huh, This is basically the way I'm doing it in other plugins too but I tried to follow the same conventions of your plugin, That being said, I will try to rewrite those using pluginManager.withPlugin
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.
Done 👍
...n/src/main/kotlin/dev/shreyaspatil/composeCompilerMetricsGenerator/plugin/ReportGenPlugin.kt
Outdated
Show resolved
Hide resolved
...hreyaspatil/composeCompilerMetricsGenerator/plugin/task/ComposeCompilerReportGenerateTask.kt
Outdated
Show resolved
Hide resolved
made the required changes and tested the plugin locally, I saw no regressions 🥳 |
Cool, now change looks good @mr3y-the-programmer. Thanks for contributing and it's definitely gonna help multi-platform devs. Kudos 🎉 I'll merge it in a separate branch and will do some cleanup, doc update and will plan release in few days. |
this PR expands the gradle plugin's support to make it work with compose multiplatform (at the very least android and jvm desktop targets should be supported now).
There are some breaking changes though, As usages of javaFile
API in the core module have been migrated to okio, and javaFile
API is exposed from some APIs' public surface, Therefore, those are affected by this change.