kotlinpoet icon indicating copy to clipboard operation
kotlinpoet copied to clipboard

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

Open ForteScarlet opened this issue 1 year ago • 15 comments

#304

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!

ForteScarlet avatar Aug 08 '24 16:08 ForteScarlet

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.

Egorand avatar Aug 08 '24 20:08 Egorand

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~

ForteScarlet avatar Aug 08 '24 20:08 ForteScarlet

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.

JakeWharton avatar Aug 08 '24 20:08 JakeWharton

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!

ForteScarlet avatar Aug 26 '24 06:08 ForteScarlet

Thanks! Will start the review today.

Egorand avatar Aug 27 '24 09:08 Egorand

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

ForteScarlet avatar Sep 02 '24 07:09 ForteScarlet

@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 🙂

https://github.com/ComposeGears/Valkyrie/issues/180

egorikftp avatar Oct 03 '24 09:10 egorikftp

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.

JakeWharton avatar Oct 03 '24 12:10 JakeWharton

@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? 😂

ForteScarlet avatar Oct 03 '24 13:10 ForteScarlet

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.

Egorand avatar Oct 03 '24 14:10 Egorand

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.

JakeWharton avatar Oct 03 '24 21:10 JakeWharton

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 https://github.com/square/kotlinpoet/compare/43197b042b09e7a08c8b94e82823fd6c95693885...3151780b25d9aedde28fbbfbc2b5042691b783ad, in these commits, I had to state from the commits that they could not be compiled because of unresolved references.

ForteScarlet avatar Oct 04 '24 07:10 ForteScarlet

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

JakeWharton avatar Oct 04 '24 20:10 JakeWharton

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! 😊

ForteScarlet avatar Oct 14 '24 05:10 ForteScarlet

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!

Egorand avatar Oct 14 '24 08:10 Egorand