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

Add a --js-emit-wasm option and a corresponding using directive #3255

Merged
merged 8 commits into from
Nov 5, 2024

Conversation

Quafadas
Copy link
Contributor

@Quafadas Quafadas commented Oct 30, 2024

  • run ./mill -i generate-reference-doc.run in order to generate doc correctly
  • check directive tags (should vs experimental)
  • directive doc
  • test
  • pass CI

@Quafadas
Copy link
Contributor Author

Quafadas commented Oct 31, 2024

@Gedochao I think I might have gotten to the stage where I need some help / advice / review if I may once the CI has run through.

I added an emits WASM test. To actually run the WASM, I think we'd need node 22. Whilst I was rather gung-ho trampling the scala-ji-cli gha to add node 22, I'm far more hesitant about doing so for Scala CLI itself.

https://github.com/Quafadas/scala-cli/blob/cb64e000c0dbacc9e7ba831c83b85d82ba70c9b2/modules/integration/src/test/scala/scala/cli/integration/RunScalaJsTestDefinitions.scala#L327-L328

I'm proposing to not actually run the WASM emitted in this test suite, rather only test that the file exists. Execution is tested in the underlying js-cli project so it's arguably a duplication. If you feel strongly about it then I'll have to go through specifying some node versions...

The second thing, is that I noticed a test failure on windows BSP on the last time out. I don't think that is me, but I'm also not qualified to diagnose what's happening there. I think CI should go green for my set of changes. If it doesn't, I might need some help to figure out what's going on, if that's okay.

Finally, I'm not sure if any extra documentation is needed beyond what scala-cli demands as the minimal set it will pass CI with? Perhaps not, as sjrd covered that in the release notes of scala js itself?

Outside of those points, I believe this would be ready for review / merge ...

Copy link
Contributor

@Gedochao Gedochao left a comment

Choose a reason for hiding this comment

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

LGTM

@Gedochao
Copy link
Contributor

Gedochao commented Nov 5, 2024

@Quafadas thanks a lot for attending to this!

@Gedochao Gedochao changed the title WIP: Emit wasm in scala cli Add a --js-emit-wasm option and a corresponding using directive Nov 5, 2024
@Gedochao Gedochao merged commit 72c23a5 into VirtusLab:main Nov 5, 2024
78 checks passed
@Quafadas
Copy link
Contributor Author

Quafadas commented Nov 5, 2024

@Gedochao Pleasure - thanks for the support :-).

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.

2 participants