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

Apply the JS and WasmJs source sets to the main module #1992

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

ForteScarlet
Copy link
Contributor

#304

It's part of #1959 ,
move the JVM platform content from common* to jvm* and add the JS and WasmJs platform targets for the main module.

Copy link
Collaborator

@Egorand Egorand left a comment

Choose a reason for hiding this comment

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

Overall it's clear what's going on, though not super clear what the plan is and what the next steps are - left some questions, and happy to approve once it's clarified. Will also need a rebase, I just merged some changes which broke the PR, sorry!

kotlin-js-store/yarn.lock Outdated Show resolved Hide resolved
kotlinpoet/build.gradle.kts Show resolved Hide resolved
kotlinpoet/gradle.properties Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the plan with moving all of these back into the jvm source set, that we'll later extract expect code and move it back into common? And specifically for tests, I imagine most test classes will eventually end up back in common with no expect/actual versions, so is it worth moving them to jvm now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now commonMain is actually jvmMain, since it previously had only one jvm{ } source code set.
It referenced a lot of JVM code and couldn't keep it with more platform targets added.

And if we just replace them with expect/actual and TODO, I think that's still going to change a lot of content, just like the whole #1959 thing, except the other platform stuff becomes TODO for now.

My idea is to move everything to the jvm, try to make sure that each change compiles successfully and does not affect the results on the jvm platform, and then move on from there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it, I think this makes sense, thanks!

@Egorand
Copy link
Collaborator

Egorand commented Oct 14, 2024

Echoing what Jake said here, it's fine to ship incomplete or broken JS target as we don't yet have any consumers, so if moving code to the jvm source set was done mostly to avoid breaking JS, I don't think it's necessary.

@ForteScarlet
Copy link
Contributor Author

Echoing what Jake said here, it's fine to ship incomplete or broken JS target as we don't yet have any consumers, so if moving code to the jvm source set was done mostly to avoid breaking JS, I don't think it's necessary.

If we keep the existing code and change it to expect/actual all at once, then let the JS implementation be TODO and the JVM compile as usual with the same results. There will still be a lot of change.

For example:

  • Where files are involved (such as FileSpec).
  • in the companion or Class defined in the public function, and the parameter is only for the JVM (such as Annotatable.addAnnotation(Class<*>)).

Although many of them are marked Deprecated or DelicateKotlinPoetApi, we still want to keep them too.

@ForteScarlet
Copy link
Contributor Author

though not super clear what the plan is and what the next steps are

My current idea is to change the platform configuration to the multi-platform and keep all JVM effects (which is what this pr is now), then gradually migrate to common.

The steps involved are probably similar to those commits in the https://github.com/square/kotlinpoet/pull/1959/commits: gradual migration starting from Util.kt, JvmDefaultWithCompatibility.

Copy link
Collaborator

@Egorand Egorand left a comment

Choose a reason for hiding this comment

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

The strategy makes sense, thank you!

@Egorand
Copy link
Collaborator

Egorand commented Oct 14, 2024

@ForteScarlet can you please rebase, removing the commits that are not yours?

@ForteScarlet
Copy link
Contributor Author

OH! Maybe I'm generating some extra merges from the local rebase, I'll see how to fix it...

@ForteScarlet
Copy link
Contributor Author

It looks like the commits that weren't mine have left me

@Egorand Egorand added this to the 3.0 milestone Oct 15, 2024
@Egorand
Copy link
Collaborator

Egorand commented Oct 15, 2024

Sorry, I broke your change again with a recent merge - I just rebased and force pushed to fix it. Merging once the build is green as this change has no effect on the API. For context, we plan to release 2.0 shortly with a smaller subset of changes, and then follow up with another release that has full KMP support.

@Egorand Egorand merged commit e4e7a6f into square:main Oct 15, 2024
4 checks passed
@ForteScarlet
Copy link
Contributor Author

Sorry, I broke your change again with a recent merge - I just rebased and force pushed to fix it. Merging once the build is green as this change has no effect on the API. For context, we plan to release 2.0 shortly with a smaller subset of changes, and then follow up with another release that has full KMP support.

Okay, got it

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