Make the kotlinpoet module multi-platform and add source set configuration for the JS and wasmJs platforms #1959

ForteScarlet · 2024-08-08T16:35:51Z

Hi! This PR attempts to change the kotlinpoet module into a true multi-platform module: adding source code sets for platforms other than the JVM platform: JS and WasmJs. No native platforms have been added yet.

There should be no change in behaviour on the JVM platform after the change, and the unit tests all pass locally.

There are some limitations and issues with JS and wasmJs.

The kotlin-reflect in JS and WasmJs is very limited, so it's almost impossible to get valid results with KClass.asXxx.
The IDENTIFIER_REGEX in the Util.kt cannot be initialised, and you get the error: PatternSyntaxException: No such character class. (I'm not very good at regular expressions, I don't understand the problem with them.)
CodePoint related implementations are temporary on JS and WasmJS, not sure if that's correct.
isJavaIdentifierStart and isJavaIdentifierPart are not implemented on JS and WasmJs at the moment and only return default values temporarily.
formatNumericValue is not implemented on JS and WasmJs at the moment and currently returns toString itself directly, unformatted.

I have tried not to change the logic and ensure API compatibility, but I have to say it's still a lot of changes. I wonder if such a change would be allowed or accepted? I think this might simply advance #304 ?

Please review these changes and let me know whether you will be accepted or not, thanks!

Egorand · 2024-08-08T20:01:59Z

This PR is gigantic and due to that it's virtually unreviewable. Please split it up into multiple incremental commits, each of which has a clear commit message that describes what it's doing. Also, please try to avoid noisy changes such as moving function or property annotations from the same line to the previous line.

To be honest I'm also not completely sure there's value in getting the library to work in a non-Java environment, given the limitations. I'll let other contributors voice their opinions, probably worth waiting until then before you start cleaning it up and splitting into commits.

ForteScarlet · 2024-08-08T20:12:34Z

Thank you for your reply! I think running in a non-Java environment can make browser-based code generation or mobile code generation in non-Java environments easier. However, let's wait for the results of your discussion and look forward to the follow-up~

JakeWharton · 2024-08-08T20:39:34Z

I agree that JS is valuable.

I also agree that we need to do this piece by piece.

The TypeName hierarchy seems like a good candidate.

…ontents of commonMain and commonTest to the JVM source set.

…s enums and annotations

…n that works with it (there are unspecified implementations on JS and WasmJs)

… Annotatable, AnnotationSpec, CodeBlock to common There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment.

There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment.

Extracts several pieces of logic involving JVM platform types into expected functions. There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment. Migrate ParameterSpec to common. There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment.

Restored some of the functions in Util.kt that needed to wait for CodeBlock migration before they could be used. Migrate several internal factory functions in TypeVariableName companion objects to the platform (TypeVariableName.jvm.kt). There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment.

There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment.

Migrate several public functions with platform types in FunSpec companion objects to the platform (FunSpec.jvm.kt) There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment.

There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment.

Restore a couple of internal functions in Util.kt that need to wait for KModifier to migrate. There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment.

Migrate some APIs involved in operating platform types to the platform (FileSpec.jvm.kt) There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment.

Change the logic for initializing NO_PACKAGE to what is expected by the platform, since you can't use String's constructor directly in wasmJs.

Add ClassName type properties (OptionalExpectationAnnotations.kt) to the relevant optional platform annotation (e.g., JvmName) instead of using `::class` directly.

…hey could be used.

Apply spotless

…APIs.

ForteScarlet · 2024-08-26T06:33:43Z

Hey! A while ago, I split and re-pushed the commit message as much as possible (I think I forgot to mention it here, sorry). The current changes should have the vast majority implemented via KMP, and the behavior on the JVM platform should be no different than before.

Some of the content that may be problematic in the platform implementation, ambiguous, or I'm not quite sure how to implement it in the platform, I've marked with a TODO.

Feel free to review them and let me know if there's anything I can do, thanks!

…he WasmJs platform.

Egorand · 2024-08-27T09:04:35Z

Thanks! Will start the review today.

Egorand

(reviewed first 5 commits so far)

kotlinpoet/build.gradle.kts

Egorand · 2024-08-27T09:07:58Z

kotlinpoet/build.gradle.kts

+      }
+    }
+    binaries.library()
+


nit: delete empty line.

kotlinpoet/build.gradle.kts

Egorand · 2024-08-27T09:11:36Z

kotlinpoet/build.gradle.kts

+    wasmJsMain {
+      dependsOn(nonJvmMain)
+    }
+


nit: remove empty line.

kotlinpoet/gradle.properties

Egorand · 2024-08-27T09:24:33Z

kotlinpoet/src/nonJvmMain/kotlin/com/squareup/kotlinpoet/Util.nonJvm.kt

+}
+
+/*
+* A character may be part of a Java identifier if any of the following conditions are true:


nit: fix indentation.

Egorand · 2024-08-27T09:24:42Z

kotlinpoet/src/nonJvmMain/kotlin/com/squareup/kotlinpoet/Util.nonJvm.kt

+* - it is a non-spacing mark
+* isIdentifierIgnorable returns true for the character
+ */
+


nit: remove empty line.

Egorand · 2024-08-27T09:25:22Z

kotlinpoet/src/nonJvmMain/kotlin/com/squareup/kotlinpoet/Util.nonJvm.kt

+}
+
+internal fun Char.isIdentifierIgnorable(): Boolean {
+  /*


nit: non-documentation comments don't need the /** */ syntax, can just use //.

kotlinpoet/src/jvmMain/kotlin/com/squareup/kotlinpoet/jvm/alias/JvmAnnotationMirror.jvm.kt

kotlinpoet/src/nonJvmMain/kotlin/com/squareup/kotlinpoet/jvm/alias/JvmModifier.nonJvm.kt

JakeWharton · 2024-08-27T16:54:42Z

kotlinpoet/build.gradle.kts

+    browser {
+      testTask {
+        useKarma {
+          useChromeHeadless()
+        }
+      }
+    }
+    nodejs {
+      testTask {
+        useMocha()
+      }
+    }


There's no need to run the tests in both the browser and on Node. Just Node is fine.

JakeWharton · 2024-08-27T16:57:20Z

kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/ClassName.kt

+/** Returns the class name for `element`. */
+@DelicateKotlinPoetApi(
+  message = "Element APIs don't give complete information on Kotlin types. Consider using" +
+    " the kotlinpoet-metadata APIs instead."
+)
+public expect fun JvmTypeElement.asClassName(): ClassName


Things that are JVM-specific should not be in common. Moreover, kotlinpoet-metadata is JVM-only due to kotlin-metadata being JVM-only.

JakeWharton · 2024-08-27T16:59:47Z

kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/Closeable.kt

+ */
+package com.squareup.kotlinpoet
+
+public expect interface Closeable : AutoCloseable


We should use AutoCloseable on all platforms and only add the JVM's Closeable where needed. We don't want to put a type in our public API for just doing this, though.

I didn't realize that the types that implement Closeable are internal types, you're right, it doesn't seem necessary to define an additional Closeable

…oseable` directly instead.

ForteScarlet · 2024-09-02T07:23:49Z

Changes have been made to the needed adjustments mentioned above. Feel free to let me know of any other reviews after that~

egorikftp · 2024-10-03T09:31:38Z

@JakeWharton @Egorand Could you please take a look the PR?
I'm developing plugin for converting xml/svg into Compose ImageVector and I want to make a WASM version, but kotlinpoet not works on wasm target yet 🙂

ComposeGears/Valkyrie#180

JakeWharton · 2024-10-03T12:58:00Z

I don't think this PR is reviewable. We need to start with something smaller in scope like the TypeName hierarchy by itself. There's way too much API surface here to do at once.

ForteScarlet · 2024-10-03T13:09:33Z

@JakeWharton I think I may have misunderstood a bit before. I thought I needed to split the content of this PR into more defined commits based on the changes, but now it seems I need to submit the PRs for each "small part" of the changes step-by-step, right? 😂

Egorand · 2024-10-03T14:11:58Z

Yeah, that was the original suggestion, but it seems it'll be really hard to review it all as a single PR even broken down into individual commits. Agree we should try to merge as many bits as possible independently.

JakeWharton · 2024-10-03T21:44:31Z

I definitely mentioned only doing something small like TypeName first somewhere, but I can't find where. It's too hard to judge whether this is correct or not since it's so massive, and it'll take forever for us to review and then iterate on the feedback and such.

ForteScarlet · 2024-10-04T07:40:49Z

Well, I'll see what I can continue to do afterward based on some smaller parts.

However, from what I remember from this PR comment, when I wanted to start working in steps from TypeName, I found that TypeName, WildcardTypeName, ParameterizedTypeName, CodeWriter, etc. have references to each other and some associations with each other, making it difficult to narrow down what would be affected by one change. It's hard to narrow down what's affected.
Referring to 43197b0...3151780, in these commits, I had to state from the commits that they could not be compiled because of unresolved references.

JakeWharton · 2024-10-04T20:12:01Z

Since we have no users on other targets, we can expect/actual odd dependencies and use TODO() implementations for now.

ForteScarlet · 2024-10-14T05:17:49Z

I submitted a more ‘smaller’ change #1992 a while ago following the commit steps in this pr.
If you have any suggestions or need me to do, please feel free to let me know, thanks! 😊

Egorand · 2024-10-14T08:59:48Z

I submitted a more ‘smaller’ change #1992 a while ago following the commit steps in this pr. If you have any suggestions or need me to do, please feel free to let me know, thanks! 😊

Yep yep, thanks for that, planning to review this week - apologies for the delay!

Apply the JS and WasmJs source sets to the main module and move the c…

ae89e37

…ontents of commonMain and commonTest to the JVM source set.

ForteScarlet marked this pull request as draft August 10, 2024 08:10

ForteScarlet added 25 commits August 10, 2024 16:27

Migrating the Util.kt to common

a46a5a7

Migrating JvmDefaultWithCompatibility to common

4d24217

Creating type aliases for JVM platform compatibility

52068af

Migrate types that have no or little use for platform content, such a…

02569d1

…s enums and annotations

Migrate NameAllocator to common and add a new CodePoint implementatio…

92495f2

…n that works with it (there are unspecified implementations on JS and WasmJs)

Add the yarn.lock and apply spotless in stages

511e8e8

Migrate OriginatingElementsHolder.kt to common

6a9198d

Migrate Taggable.kt to common

43197b0

Migrate TypeName, WildcardTypeName, ParameterizedTypeName, ClassName,…

5259b9c

… Annotatable, AnnotationSpec, CodeBlock to common There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment.

Migrate Documentable to common

11522f1

There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment.

Migrate ContextReceivable and Dynamic to common

8c8ddeb

There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment.

Migrate MemberName to common

e31e57c

There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment.

Migrate TypeAliasTag to common

462aa07

There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment.

Migrate LineWrapper to common

80fb80d

There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment.

Migrate LambdaTypeName to common.

3dfd12f

There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment.

Migrate FunSpec and PropertySpec to common.

d5be4e7

Migrate several public functions with platform types in FunSpec companion objects to the platform (FunSpec.jvm.kt) There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment.

Migrate MemberSpecHolder to common.

16a0cf0

There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment.

Migrate TypeSpec and TypeSpecHolder to common.

4725895

Restore a couple of internal functions in Util.kt that need to wait for KModifier to migrate. There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment.

Migrate FileSpec to common.

e158bba

Migrate some APIs involved in operating platform types to the platform (FileSpec.jvm.kt) There are some related but unmigrated types (such as CodeWriter), and this commit cannot be compiled at the moment.

Migrate CodeWriter to common.

3151780

Change the logic for initializing NO_PACKAGE to what is expected by the platform, since you can't use String's constructor directly in wasmJs.

Migrate JvmAnnotations.kt to common.

63440e7

Add ClassName type properties (OptionalExpectationAnnotations.kt) to the relevant optional platform annotation (e.g., JvmName) instead of using `::class` directly.

Restore functions in Taggable that waited for type migration before t…

12b6818

…hey could be used.

Restoring a Temporary TODO in AnnotationSpec

e44b6fb

ForteScarlet added 4 commits August 10, 2024 21:51

Fix some of the noisy changes.

97755a2

Create an expected typealias Closeable for java.io.Closeable

7afe913

Apply spotless

Adjust and optimize the structure of CodePoint and the logic of some …

dcfab3f

…APIs.

Edit changelog.md

ca71f03

ForteScarlet force-pushed the multiplatform branch from 9d84145 to ca71f03 Compare August 10, 2024 14:36

ForteScarlet marked this pull request as ready for review August 10, 2024 14:38

Try to edit String.isIdentifier in Util.kt to make it usable on t…

9133127

…he WasmJs platform.

Egorand reviewed Aug 27, 2024

View reviewed changes

JakeWharton reviewed Aug 27, 2024

View reviewed changes

ForteScarlet added 5 commits August 29, 2024 13:40

Fix some blank lines, typos and indentation issues

36c8e23

Remove the browser's tests configuration

9343ea2

Move JvmTypeElement.asClassName to JVM platform

71a69af

Clean up the standalone Closeable type and implement `kotlin.AutoCl…

8eed20b

…oseable` directly instead.

Remove the browser's tests configuration for js platform

584fe49

ForteScarlet mentioned this pull request Oct 5, 2024

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

Merged

ForteScarlet mentioned this pull request Oct 15, 2024

Migrating the Util.kt and KModifier from JVM to common #2006

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the kotlinpoet module multi-platform and add source set configuration for the JS and wasmJs platforms #1959

Make the kotlinpoet module multi-platform and add source set configuration for the JS and wasmJs platforms #1959

ForteScarlet commented Aug 8, 2024

Egorand commented Aug 8, 2024

ForteScarlet commented Aug 8, 2024

JakeWharton commented Aug 8, 2024

ForteScarlet commented Aug 26, 2024

Egorand commented Aug 27, 2024

Egorand left a comment

Egorand Aug 27, 2024

Egorand Aug 27, 2024

Egorand Aug 27, 2024

Egorand Aug 27, 2024

Egorand Aug 27, 2024

JakeWharton Aug 27, 2024

JakeWharton Aug 27, 2024

JakeWharton Aug 27, 2024

ForteScarlet Aug 29, 2024

ForteScarlet commented Sep 2, 2024

egorikftp commented Oct 3, 2024

JakeWharton commented Oct 3, 2024

ForteScarlet commented Oct 3, 2024

Egorand commented Oct 3, 2024

JakeWharton commented Oct 3, 2024

ForteScarlet commented Oct 4, 2024

JakeWharton commented Oct 4, 2024

ForteScarlet commented Oct 14, 2024

Egorand commented Oct 14, 2024

Make the kotlinpoet module multi-platform and add source set configuration for the JS and wasmJs platforms #1959

Are you sure you want to change the base?

Make the kotlinpoet module multi-platform and add source set configuration for the JS and wasmJs platforms #1959

Conversation

ForteScarlet commented Aug 8, 2024

Egorand commented Aug 8, 2024

ForteScarlet commented Aug 8, 2024

JakeWharton commented Aug 8, 2024

ForteScarlet commented Aug 26, 2024

Egorand commented Aug 27, 2024

Egorand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ForteScarlet commented Sep 2, 2024

egorikftp commented Oct 3, 2024

JakeWharton commented Oct 3, 2024

ForteScarlet commented Oct 3, 2024

Egorand commented Oct 3, 2024

JakeWharton commented Oct 3, 2024

ForteScarlet commented Oct 4, 2024

JakeWharton commented Oct 4, 2024

ForteScarlet commented Oct 14, 2024

Egorand commented Oct 14, 2024