kotlinpoet
kotlinpoet copied to clipboard
Make the kotlinpoet module multi-platform and add source set configuration for the JS and wasmJs platforms
#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-reflectin JS and WasmJs is very limited, so it's almost impossible to get valid results withKClass.asXxx. - The
IDENTIFIER_REGEXin theUtil.ktcannot 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.) CodePointrelated implementations are temporary on JS and WasmJS, not sure if that's correct.isJavaIdentifierStartandisJavaIdentifierPartare not implemented on JS and WasmJs at the moment and only return default values temporarily.formatNumericValueis not implemented on JS and WasmJs at the moment and currently returnstoStringitself 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!
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.
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~
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.
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!
Thanks! Will start the review today.
Changes have been made to the needed adjustments mentioned above. Feel free to let me know of any other reviews after that~
@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
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 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? 😂
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.
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.
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.
Since we have no users on other targets, we can expect/actual odd dependencies and use TODO() implementations for now.
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! 😊
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!