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

Enable support for Kotlin/JS IR backend and introduce js_export parameter #147

Merged
merged 12 commits into from
Jun 6, 2021

Conversation

itegulov
Copy link
Contributor

Fixes #136

I have tested js_export on my company's projects and it works pretty well. Obviously the resulting .d.ts declaration has some "red" parts as @JsExport does not support Kotlin collections, but overall it is still usable.

Let me know what you think and thanks for this amazing library!

Copy link
Collaborator

@garyp garyp left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @itegulov!

It looks like the build is failing in CI with a StackOverflowError from the Kotlin compiler. Unfortunately we've run into this many times with the new IR backend used by Kotlin/Native (and now available on JS and JVM too). It seems that the new IR backend is still buggy and certain Kotlin constructs in pbandk's source code will cause it to either use an inordinate amount of memory during compilation, or will cause it to get into an infinite loop. Both of which manifest as out-of-memory or stack overflow errors. You could try tweaking the jvmargs in gradle.properties to increase the memory available and see if that helps. If you can't replicate this when building on your own machine, try limiting the amount of memory available to the process to under 7GB. That's how much memory the CI job runs with.

I also see a bunch of warnings in the build log about non-exported types. Is this anything we need to worry about? E.g.:

w: /home/runner/work/pbandk/pbandk/runtime/src/commonMain/kotlin/pbandk/ExtendableMessage.kt: (10, 38): Exported declaration uses non-exportable parameter type: FieldDescriptor<M, T>

conformance/js/run.sh Outdated Show resolved Hide resolved
conformance/lib/build.gradle.kts Show resolved Hide resolved
runtime/src/commonMain/kotlin/pbandk/Message.kt Outdated Show resolved Hide resolved
@itegulov itegulov force-pushed the 136-kotlin-js-ir-backend branch from 5216755 to 1dfb590 Compare May 4, 2021 02:25
@itegulov
Copy link
Contributor Author

itegulov commented May 4, 2021

@garyp Thanks for the review. StackOverflowError also happens occasionally on my local machine. Bumping -Xss to 512m seems to have resolved the issue for me locally, lets see how CI likes it... Also, due to the new Github rules and me being a first-time contributor, you need to give my PR a manual approval on Github Actions before any Actions workflows will run (see more here: https://github.blog/2021-04-22-github-actions-update-helping-maintainers-combat-bad-actors/).

I also see a bunch of warnings in the build log about non-exported types. Is this anything we need to worry about? E.g.:

Unfortunately, this is one of the short-comings of the @JsExport annotation: it does not fully support generics, Kotlin collections and any other custom classes that are not annotated with @JsExport. There is no way to get rid of those warnings completely without re-designing the API to be fully compatible with what @JsExport expects. So my take here is that it we should just ignore those warnings and hopefully @JsExport will be improved in the future. The generated Typescript declarations are still usable anyway, you just need to write a few custom utilities to e.g. convert between JS array and kotlin.collections.List or convert between JS objects and kotlin.collections.Map.

@itegulov itegulov force-pushed the 136-kotlin-js-ir-backend branch from 1dfb590 to bcdf95b Compare May 12, 2021 10:32
gradle.properties Outdated Show resolved Hide resolved
itegulov added 4 commits May 15, 2021 13:59
Module 'fs' is not available for browser environments which is a
fatal error with IR compiler backend. Since we are using
conformance tests specifically with NodeJS there is no reason to
cross-build it for browsers anyway.
@itegulov itegulov force-pushed the 136-kotlin-js-ir-backend branch from bcdf95b to ccb67e4 Compare May 15, 2021 04:55
@itegulov itegulov requested a review from garyp May 28, 2021 02:41
@itegulov
Copy link
Contributor Author

@garyp the PR is ready from my perspective, could you take another look please?

examples/browser-js/app/build.gradle.kts Outdated Show resolved Hide resolved
runtime/src/commonMain/kotlin/pbandk/Export.kt Outdated Show resolved Hide resolved
conformance/lib/build.gradle.kts Show resolved Hide resolved
@garyp
Copy link
Collaborator

garyp commented Jun 5, 2021

A couple other things:

  • CI is failing because you need to re-generate the code for the protobuf types that are bundled with pbandk and add those updates to your PR. This is the command to run:
./gradlew :runtime:generateWellKnownTypes \
    :runtime:generateTestTypes \
    :protoc-gen-kotlin:protoc-gen-kotlin-lib:generateProto \
    :conformance:conformance-lib:generateProto
  • I ran the above command locally and then tried rebuilding the pbandk conformance tests and got a couple compiler errors:
> Task :conformance:conformance-lib:compileKotlinJs FAILED
e: /Users/gary/mydata/code/streem/pbandk/conformance/lib/src/commonMain/kotlin/pbandk/conformance/pb/test_messages_proto2.kt: (2571, 1): Declaration of such kind (extension property) cant be exported to JS
e: /Users/gary/mydata/code/streem/pbandk/conformance/lib/src/commonMain/kotlin/pbandk/conformance/pb/test_messages_proto2.kt: (2572, 5): JavaScript name (<get-extensionInt32>) generated for this declaration clashes with another declaration: fun <get-extensionInt32>(): FieldDescriptor<TestAllTypesProto2, Int?>
e: /Users/gary/mydata/code/streem/pbandk/conformance/lib/src/commonMain/kotlin/pbandk/conformance/pb/test_messages_proto2.kt: (2574, 1): JavaScript name (<get-extensionInt32>) generated for this declaration clashes with another declaration: fun TestAllTypesProto2.<get-extensionInt32>(): Int?

For the first error: I'm not sure if there's a way to tell Kotlin not to export a particular declaration when using @file:pbandk.Export at the file level? If there isn't, then we might need to drop the file-level annotation and go back to annotating each declaration that needs to be exported in the generated code.

For the second error: It's these two declarations that are conflicting with each other:

val pbandk.conformance.pb.TestAllTypesProto2.extensionInt32: Int? 
    get() = getExtension(pbandk.conformance.pb.extensionInt32)

val extensionInt32 = pbandk.FieldDescriptor(
    ...
)

They're both defining a getter on a field named extensionInt32. The first getter will have a receiver parameter in Javascript, whereas the second one won't, but they're otherwise named the same. So one of them will need an alternative name via @JsName. These two declarations are generated inside of the CodeGenerator.writeExtension() method.

itegulov added 3 commits June 5, 2021 19:21
Renamed pbandk.Name to pbandk.JsName, annotated it with @PublicForGeneratedCode
and improved browser-js example gralde configuration
@itegulov
Copy link
Contributor Author

itegulov commented Jun 5, 2021

@garyp Unfortauntely, there does not seem to be a way to exclude declarations form a file-wide @JsExport. I have made it so that all top-level classes, orDefault methods and extension field descriptors are marked with @JsExport.

They're both defining a getter on a field named extensionInt32. The first getter will have a receiver parameter in Javascript, whereas the second one won't, but they're otherwise named the same. So one of them will need an alternative name via @JsName. These two declarations are generated inside of the CodeGenerator.writeExtension() method.

Actually, there is no need to give one of them a different name since the first one is an extension property that cannot be exported anyway.

@itegulov itegulov requested a review from garyp June 5, 2021 14:42
Copy link
Collaborator

@garyp garyp left a comment

Choose a reason for hiding this comment

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

Thanks again for the improvement!

@garyp garyp merged commit aceb5a7 into streem:master Jun 6, 2021
@garyp garyp added this to the 0.12.0 milestone Sep 25, 2021
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.

Enable support for new Kotlin JS IR backend
2 participants