Wasm #315

igoriakovlev · 2023-11-15T13:55:04Z

This is WasmJs target implementation.

We update kotlin version to 1.9.20 and implement WasmJs target similar to JS target implementation.

The experimental repository dependency will be removed before merging and replaced with maven central as soon as kotlinx.serialization for WasmJs target become published.

core/build.gradle.kts

core/jsAndWasmShared/src/Date.kt

gradle.properties

core/jsAndWasmShared/src/PlatformSpecifics.kt

core/build.gradle.kts

core/jsAndWasmShared/src/PlatformSpecifics.kt

core/wasmJs/src/PlatformSpecifics.kt

build.gradle.kts

core/build.gradle.kts

dkhalanskyjb · 2023-11-22T09:50:59Z

core/common/src/serializers/DateTimeUnitSerializers.kt

@@ -21,8 +21,10 @@ import kotlin.reflect.KClass
 */
 public object TimeBasedDateTimeUnitSerializer: KSerializer<DateTimeUnit.TimeBased> {

-    override val descriptor: SerialDescriptor = buildClassSerialDescriptor("TimeBased") {
-        element<Long>("nanoseconds")
+    override val descriptor: SerialDescriptor by lazy {


If this lazy works around some bug, please accompany each of its usages with a comment with the corresponding issue number.

This bug observable only for wasm target (which seems is more strict for nullability in this case). Could you please to suggest how to file such bug and where.

I don't know how nullability comes into play here: these are all not null and don't even depend on one another.

I'd file this under https://kotl.in/issue, specifying that the behavior is different on wasm and crashes the code that works on all other platforms.

The initialisation sequence reads null value from not-null uninitialised property. Here, I put the code into lazy block to prevent reading uninitialised property. The bug exists at all platforms but seems it is observable only on wasm.

I feel we're speaking past each other.

The bug exists at all platforms but seems it is observable only on wasm.

The way I'm reading this, if this bug never surfaces on other platforms, then we can't call it a bug. Kotlin/Wasm behaves differently, making code that always works correctly on other platforms buggy. This is a problem with Kotlin/Wasm and needs to be reported.

Or did you mean that you can cause this code to misbehave on other platforms somehow, we just never encountered it, and there actually is a bug in the library code that's fixed by lazy? In what circumstances can it happen?

Either way, could you elaborate on what the bug is? At which point is a null value read from a non-null property?

I do not think this is Kotlin/Wasm compiler bug.

Ok, where is this difference coming from then? Is the serialization library using expect/actual in this part to have different code on different platforms and uses a different implementation from everything else on Kotlin/Wasm?

The fact that at all backends except wasm the behaviour is different does not mean that this is a kotlin/wasm bug, especially for the code that uses frontend suppressors.

But anyway, I will review what is happening in detail here on the next week and try to localise exact problem.

I found the difference of implementation for associated objects in the wasm that triggered the problem.
It was a difference of associated objects initialization that was not specified and tested.
In a matter of same behavior for different targets I have filed a bug for kotlin/wasm stdlib.
As far, to workaround this difference, I put lazy for all serialiser constructor initialisers in DateTime with a comment.

Good job figuring this one out!

dkhalanskyjb · 2023-11-22T10:03:19Z

core/jsAndWasmShared/src/LocalDate.kt

-            }.let(::LocalDate)
+            jsTry {
+                when (unit) {
+                    is DateTimeUnit.DayBased -> this.value.plusDays(value.toDouble() * unit.days)


The jsTry change is extremely intrusive and pollutes all JS code. If I understand correctly what's going on, this is a measure to support the exceptions thrown from pure JS (not Kotlin/JS) code, which seems incompatible with how Kotlin/WASM treats exceptions. Is this correct?

If so, we'd need to remember to surround all the JS code we call with such checks. It is easy to miss, and I don't even know what will happen if we do miss this. How badly will something break? This question needs to be answered in form of a comment somewhere in the code, no matter what solution we end up with.

Here's an idea of how this could be done in a more robust manner:

// jsAndWasmShared internal expect class JodaTimeLocalDate { fun plusDays(days: Number): Number // ... } class LocalDateTime(value: JodaTimeLocalDate) // instead of `value: jtLocalDate` // js internal actual typealias JodaTimeLocalDate = jtLocalDate // wasm internal actual value class JodaTimeLocalDate(val value: jtLocalDate) { fun plusDays(days: Number): Number = JodaTimeLocalDate(jsTry { value.plusDays(days) }) // ... }

This way, the shared source set will stay almost the same, and all WASM-specific logic will be in one place where it's easy to review and spot if something's wrong.

What do you think? Is this approach viable?

Yeah, when you commonize a code between two platforms with different interop it always looks tricky. This is a price for commonization.

Yes. In kotlin.wasm platform we have no ability (so far) to catch exceptions thowed from javascript code. This is why I put special wrapper to catch these exceptions inside js world and rethrow it as wasm exceptions in wasm world. For js platform its just a nop.
When I was writing this I had a simple mental rule - every time you want to try-catch exception from js library you should wrap jsTry this block.

Your suggest is indeed more readable from a perspective of jsTry but one more indirection level makes a mess and boilerplate.

There is several cons for this:

Now we have two Instant types - DateTime.Instant and jsJoda.Instant external. With this approach we will have three similar types - DateTime.Instant, jsJoda.Instant and WasmWrapper.Instant.

Everywhere we use joda externals we need now make 2 unbox operations, like jtDuration.between(other.value.jodaInstant, this.value.jodaInstant).

Everywhere we pass jsJoda declarations to DateTime declarations we need to pack twice.

You need to overlap almost all Joda external hierarchy.

It does not solves major problem, because you still need to know which methods should be wrapped with jsTry and which is not.

Now you can formulate a common rule - any place I put try-catch for js exception should be wrapped with jsTry also. Not sure that wrappers can do better.

But:
If you still think that it can do better, I can try to do that but for now it does't seem any better for me.

Your suggest is indeed more readable from a perspective of jsTry but one more indirection level makes a mess and boilerplate.

In the common code, we replace one no-op indirection (jsTry) with another (a type alias), a more readable one. I think the number of indirections stays the same.

Now we have two Instant types

We had two Instant types: the user-visible facade (kotlinx.datetime.Instant) and the implementation (Instant from JsJoda). Now we have three Instant types: the user-visible facade (kotlinx.datetime.Instant), the adapter of platform-specific code (kotlinx.datetime.JodaTimeInstant), and the platform-specific implementation (no-op type alias on JS, a wrapper on WASM). All three entities seem to have their places.

You need to overlap almost all Joda external hierarchy.

The good thing is that this only needs to be done once, and this code only needs to be rarely touched. On the other hand, we edit the shared code fairly often. Its clarity is worth a lot.

2 unbox operations

pack twice

Won't value class work here? Also, is this only a concern performance-wise?

It does not solves major problem, because you still need to know which methods should be wrapped with jsTry and which is not.

It does: now we need to know to wrap only in the place of definition, not on every call.

You cant make typealias actual in js for these external declarations. You will be forced to hold this fierarchy both for js and wasm.

I'm ok with the current jsTry approach, we could refactor it later if we find it disadvantageous.

Some miscommunication probably happened: I agreed not to have this refactoring. Sorry if I worded this incorrectly!

Ohh, I missed this post. :(
So, need I revert this?

Yes, please do, if you want this review to be completed sooner than later

@ilya-g, why do you believe that this approach would take longer to review? With this separation, we barely have to look at the shared code; the changes there are trivial. We only have to look through a small list of definitions and check that they are wrapped.

What does make this PR difficult to review now is the amount of code that's touched but not meaningfully changed. This needs to be fixed. After that, the changes would be self-contained.

I reduced unnecessary touched code. If you still want me to remove wrappers then I will.

core/jsAndWasmShared/src/TimeZone.kt

core/jsAndWasmShared/src/LocalDateTime.kt

core/js/test/JsConverterTest.kt

ilya-g · 2023-11-28T05:19:40Z

master is updated to Kotlin 1.9.21, please rebase on it.

ilya-g · 2023-11-28T05:36:56Z

core/build.gradle.kts

+
+// Disable intermediate sourceSet compilation because we do not need js-wasmJs artifact
+tasks.configureEach {
+    if (name == "compileJsAndWasmSharedMainKotlinMetadata") {


While it disables the compilation, the information about the corresponding shared source set is still present in kotlin-project-structure-metadata.json embedded into common klib jar.
So it needs to be checked what effects it would have in IDE in projects having similar shared source sets.

I used a such approach for all libraries and did't find any problems with it so far. The only problem that I found so far on the K1 IDE now reports error that it is no actual for JS and Wasm targets (in a same time it found these actuals in the shared sourceSet).

build.gradle.kts

core/jsAndWasmShared/src/JSJodaExceptions.kt

core/jsAndWasmShared/src/LocalDateTime.kt

core/wasmJs/src/JSJodaExceptions.kt

core/build.gradle.kts

core/wasmJs/src/JSJodaExceptions.kt

dkhalanskyjb · 2023-11-30T10:05:47Z

core/common/src/serializers/DateTimeUnitSerializers.kt

@@ -21,8 +21,10 @@ import kotlin.reflect.KClass
 */
 public object TimeBasedDateTimeUnitSerializer: KSerializer<DateTimeUnit.TimeBased> {

-    override val descriptor: SerialDescriptor = buildClassSerialDescriptor("TimeBased") {
-        element<Long>("nanoseconds")
+    override val descriptor: SerialDescriptor by lazy {


Good job figuring this one out!

igoriakovlev requested a review from ilya-g November 15, 2023 13:55

ilya-g reviewed Nov 15, 2023

View reviewed changes

core/build.gradle.kts Outdated Show resolved Hide resolved

core/build.gradle.kts Outdated Show resolved Hide resolved

core/jsAndWasmShared/src/Date.kt Outdated Show resolved Hide resolved

gradle.properties Outdated Show resolved Hide resolved

igoriakovlev requested a review from ilya-g November 15, 2023 14:47

igoriakovlev force-pushed the wasm branch from 8e55747 to d3260fd Compare November 20, 2023 18:14

ilya-g requested changes Nov 22, 2023

View reviewed changes

dkhalanskyjb requested changes Nov 22, 2023

View reviewed changes

igoriakovlev requested review from dkhalanskyjb and ilya-g November 22, 2023 15:00

ilya-g reviewed Nov 22, 2023

View reviewed changes

core/js/test/JsConverterTest.kt Outdated Show resolved Hide resolved

bashor mentioned this pull request Nov 24, 2023

Support Kotlin/Wasm target for JS environments (browser, nodejs) #267

Closed

ilya-g reviewed Nov 28, 2023

View reviewed changes

dkhalanskyjb requested changes Nov 28, 2023

View reviewed changes

build.gradle.kts Outdated Show resolved Hide resolved

core/jsAndWasmShared/src/JSJodaExceptions.kt Outdated Show resolved Hide resolved

core/jsAndWasmShared/src/LocalDateTime.kt Outdated Show resolved Hide resolved

core/wasmJs/src/JSJodaExceptions.kt Outdated Show resolved Hide resolved

dkhalanskyjb assigned igoriakovlev Nov 29, 2023

dkhalanskyjb added this to the 0.5.0 milestone Nov 29, 2023

igoriakovlev force-pushed the wasm branch 2 times, most recently from 8cdb53f to ca05466 Compare November 29, 2023 16:39

ilya-g reviewed Nov 29, 2023

View reviewed changes

core/build.gradle.kts Outdated Show resolved Hide resolved

ilya-g reviewed Nov 29, 2023

View reviewed changes

core/build.gradle.kts Outdated Show resolved Hide resolved

igoriakovlev requested review from dkhalanskyjb, sandwwraith and ilya-g November 29, 2023 19:01

dkhalanskyjb reviewed Nov 30, 2023

View reviewed changes

dkhalanskyjb approved these changes Nov 30, 2023

View reviewed changes

sandwwraith removed their request for review November 30, 2023 16:05

ilya-g approved these changes Dec 1, 2023

View reviewed changes

igoriakovlev force-pushed the wasm branch from d6afe30 to 52693c1 Compare December 1, 2023 14:06

igoriakovlev added 2 commits December 1, 2023 15:22

Move js files into commonJs source set

9988822

Implement WasmJs target

b3536e8

ilya-g force-pushed the wasm branch from 52693c1 to b3536e8 Compare December 1, 2023 14:27

ilya-g merged commit 841b0a8 into master Dec 1, 2023
1 check passed

ilya-g deleted the wasm branch December 1, 2023 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wasm #315

Wasm #315

igoriakovlev commented Nov 15, 2023 •

edited

Loading

dkhalanskyjb Nov 22, 2023

igoriakovlev Nov 22, 2023

dkhalanskyjb Nov 22, 2023

igoriakovlev Nov 22, 2023 •

edited

Loading

dkhalanskyjb Nov 23, 2023

igoriakovlev Nov 24, 2023

dkhalanskyjb Nov 24, 2023

igoriakovlev Nov 24, 2023 •

edited

Loading

igoriakovlev Nov 29, 2023

dkhalanskyjb Nov 30, 2023

dkhalanskyjb Nov 22, 2023

igoriakovlev Nov 22, 2023 •

edited

Loading

dkhalanskyjb Nov 22, 2023

igoriakovlev Nov 22, 2023 •

edited

Loading

ilya-g Nov 22, 2023

dkhalanskyjb Nov 29, 2023

igoriakovlev Nov 29, 2023

ilya-g Nov 29, 2023

dkhalanskyjb Nov 29, 2023

igoriakovlev Nov 29, 2023

ilya-g commented Nov 28, 2023

ilya-g Nov 28, 2023

igoriakovlev Nov 29, 2023

dkhalanskyjb Nov 30, 2023

Wasm #315

Wasm #315

Conversation

igoriakovlev commented Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igoriakovlev Nov 22, 2023 • edited Loading

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

igoriakovlev Nov 24, 2023 • edited Loading

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

igoriakovlev Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igoriakovlev Nov 22, 2023 • edited Loading

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

ilya-g commented Nov 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igoriakovlev commented Nov 15, 2023 •

edited

Loading

igoriakovlev Nov 22, 2023 •

edited

Loading

igoriakovlev Nov 24, 2023 •

edited

Loading

igoriakovlev Nov 22, 2023 •

edited

Loading

igoriakovlev Nov 22, 2023 •

edited

Loading