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

Implement JS target (experimental) #437

Merged
merged 8 commits into from
Jul 22, 2023

Conversation

krzema12
Copy link
Contributor

@krzema12 krzema12 commented Jun 16, 2023

This change provides a working JS version of kaml (tested through a separate demo consumer Gradle module).

However, it's not yet ready to be shipped because:

  • the Maven Central publishing is likely to require an adjustment. We've recently configured it for snakeyaml-engine-kmp
  • there's a problem with running JS unit tests
    • DescribeSpec seems to be unsupported by the JS target
  • missing unit tests for the JS-specific parts

@charleskorn
Copy link
Owner

charleskorn commented Jul 3, 2023

Thanks for the PR @krzema12!

A couple of notes / questions:

  • I don't have time to finish this myself, and I also have no experience with Kotlin/JS, but I'm happy to provide support / feedback to anyone that's keen to pick it up
  • If snakeyaml-engine-kmp supports the JVM as well, it'd be great to use it for the JVM, as that would significantly reduce the amount of platform-specific code
  • Can you elaborate on "the Maven Central publishing is likely to require an adjustment"? As far as I'm aware, it should Just Work™️
  • What's the issue with running nested tests on JS? Can this be fixed in Kotest?

@krzema12
Copy link
Contributor Author

krzema12 commented Jul 3, 2023

If snakeyaml-engine-kmp supports the JVM as well, it'd be great to use it for the JVM, as that would significantly reduce the amount of platform-specific code

Long-term, I'm totally for it as well, but for now our KMP port is yet immature and using it already for the JVM could introduce some regressions. That's why I propose to depend on snakeyaml-engine-kmp for the JS target for the beginning, let it stabilize a bit, and in the longer term, switch completely to snakeyaml-engine-kmp.

Can you elaborate on "the Maven Central publishing is likely to require an adjustment"? As far as I'm aware, it should Just Work™️

I just remember from my early days of experimenting with KMP that publication to Maven Central required some fancy hacks, but indeed looking at kaml's build logic, it doesn't mention anything Java-specific - indeed it may just work! It would be a matter of trying it out in practice.

What's the issue with running nested tests on JS? Can this be fixed in Kotest?

See the Gradle log:

> Task :jsNodeTest
WARN: kotest-js only supports top level tests due to underlying platform limitations. 'YamlExceptionTest' has been marked as ignored
WARN: kotest-js only supports top level tests due to underlying platform limitations. 'YamlListTest' has been marked as ignored
WARN: kotest-js only supports top level tests due to underlying platform limitations. 'YamlMapTest' has been marked as ignored
WARN: kotest-js only supports top level tests due to underlying platform limitations. 'YamlNodeTest' has been marked as ignored
WARN: kotest-js only supports top level tests due to underlying platform limitations. 'YamlNullTest' has been marked as ignored
WARN: kotest-js only supports top level tests due to underlying platform limitations. 'YamlPathTest' has been marked as ignored
WARN: kotest-js only supports top level tests due to underlying platform limitations. 'YamlReadingTest' has been marked as ignored
WARN: kotest-js only supports top level tests due to underlying platform limitations. 'YamlScalarTest' has been marked as ignored
WARN: kotest-js only supports top level tests due to underlying platform limitations. 'YamlTaggedNodeTest' has been marked as ignored
WARN: kotest-js only supports top level tests due to underlying platform limitations. 'YamlWritingTest' has been marked as ignored

It comes from this place in kotest. I'm not sure yet if it's better to somehow rewrite the tests in kaml or improve kotest.


Given the above and our limited bandwidth, maybe it's fine with you to just consider merging this PR in the current form, getting some exposure for the new target and applying fixes as needed? Just be sure to review the changes in the common Kotlin code as they affect the JVM target as well.

@russellbanks
Copy link
Contributor

It comes from this place in kotest. I'm not sure yet if it's better to somehow rewrite the tests in kaml or improve kotest.

#27 (comment)
#431

@krzema12
Copy link
Contributor Author

krzema12 commented Jul 4, 2023

@russellbanks cool, I guess we could revive your PR #431!

@charleskorn
Copy link
Owner

#431 is merged, thanks for your patience!

I'm happy to merge this once the merge conflict is resolved - could you please also add a note to the readme that makes it super clear that JS support is very experimental, untested, likely to break / change etc.?

@krzema12 krzema12 changed the title Implement JS target Implement JS target (experimental) Jul 17, 2023
@krzema12 krzema12 marked this pull request as ready for review July 18, 2023 07:20
@krzema12
Copy link
Contributor Author

@charleskorn cool, I've just updated the PR, please review!

@charleskorn charleskorn merged commit 85e3aa2 into charleskorn:main Jul 22, 2023
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.

3 participants