json5k icon indicating copy to clipboard operation
json5k copied to clipboard

Support Kotlin/JS?

Open aSemy opened this issue 2 years ago • 7 comments

Currently json5k does not support Kotlin/JS, would it be possible to add it as a target?

// build.gradle.kts
plugins {
  kotlin("multiplatform")
}

kotlin {
  targets {
    js(IR) {
      browser()
      nodejs()
    }
  }
}

This was previously discussed in #1

Support for the JS target still depends on the availability of a portable String.format function.

Originally posted by @xn32 in https://github.com/xn32/json5k/issues/1#issuecomment-1465184218

Related issues:

  • https://github.com/Kotlin/kotlinx.serialization/issues/2128
  • https://youtrack.jetbrains.com/issue/KT-21644

aSemy avatar Jun 16 '23 18:06 aSemy

hey @xn32, I've had a stab at a workaround, take a look at this commit: https://github.com/aSemy/json5k/pull/1/commits/467f4a1d15a8748a0e1b2911fc672b589657ac23

There are some minor cosmetic differences. Some scientific-notation numbers are formatted with a .0, e.g. -9.0E-4, on non-JS, while on JS they're formatted without the .0, e.g. -9E-4, which I think is pretty inconsequential.

The whole PR is here: https://github.com/aSemy/json5k/pull/1. Because I'm a Gradle nerd I refactored the build config to use convention plugins and build cache, and I also bumped a few versions. Do you want me to create the PR against xn32/json5k as-is, or should I break it up into smaller PRs? I'd prefer just applying the whole PR as-is.

aSemy avatar Jun 17 '23 12:06 aSemy

Thank you for bringing this up. I agree it makes sense to think of an alternative solution to the problem.

Basically, I see two options:

  1. We modify the serialization logic to ensure that even on Kotlin/JS, the serialized form of a finite floating-point number contains a decimal point or is in scientific notation.
  2. We modify the deserialization logic so that it performs implicit conversions from integers to floating-point numbers. This can be done entirely without expect/actual definitions.

Both approaches should give us round-trip safety for floating-point numbers. The workaround from your PR corresponds to the first option. Would the second option work for you as well?

xn32 avatar Jun 17 '23 22:06 xn32

Oh that's interesting, I thought that option 2 was already implemented.

So this fails:

println(Json5.decodeFromString<Double>("5"))
// ERROR Uncaught Kotlin exception: io.github.xn32.json5k.UnexpectedValueError: floating-point number expected at position 1:1

I think both should be implemented!

The distinction between decimal and integer numbers only exists in Kotlin, and JSON5 doesn't care - numbers are numbers. So, to me it makes sense to make it a decision to make in the code.

Decoding 5 to a float also matches how KxS JSON works.

println(Json.decodeFromString<Double>("5"))
// prints: 5.0

It also makes sense to make sure the encoding is (more or less) consistent between platforms. Since JVM and Native encode floats with a trailing zero, and use E and E- for scientific notation, then if JS is the odd one out then it should also work the same.

Having an arbitrary Json5.Number #2 type would also help for situations where a number is expected, but it doesn't really matter what the content is.

aSemy avatar Jun 18 '23 08:06 aSemy

I have now pushed a version that deserializes integers as floating-point numbers to the main branch. The feature is limited to conversions from decimal integers, i.e., a hexadecimal integer is not coerced to a floating-point number. With this change, I think it is justified to add the JS target.

The current snapshot includes build artifacts for Node.js. There are still a few issues to address, including better code coverage and support for JS in the browser. However, feedback is already welcome.

xn32 avatar Jun 18 '23 21:06 xn32

Thanks! I'll try it out. Are snapshots published?

aSemy avatar Jun 19 '23 09:06 aSemy

Great, thank you! Yes, they are automatically published here.

xn32 avatar Jun 19 '23 14:06 xn32

Good news: I've tried it out and I've got some basic tests that use JSON5 passing on both K/N and K/JS!

aSemy avatar Jun 25 '23 09:06 aSemy