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 Scala.js support to core and integration modules #815

Merged
merged 12 commits into from
Nov 12, 2020

Conversation

sbrunk
Copy link
Contributor

@sbrunk sbrunk commented Oct 26, 2020

This is a new attempt to add Scala.js support to Tapir (#98). I started with core and integration modules: cats, refined and all JSON integrations that are available for Scala.js. The goal is to get tapir-sttp-client working on JS eventually.

Why not continue on #351? I tried but the introduction of sbt-projectmatrix has changed the build structure a lot so it was easier to start fresh.

I've already done on some prerequisite work:

We also need Scala.js 1.3.0 because the Scala.js developers have recently added an implementation of String.toLowerCase(Locale locale) which is required by sttp-model.

I had to disable 8 of 172 tests in coreJS because they only work on the JVM for various reasons (i.e. file access or using the compiler API). The remaining ones pass except for MatchTypeTest because of the different JS semantics wrt. primitive number types. I'm not sure what to do about that.

Similarly, a few tests in JSON libraries are disabled for JS because there are some missing things i.e. regarding Date.

One thing I'm still unsure about is how to do file support for Scala.js. For now i've just disabled the File and Path codecs for JS (via a CodecExtensions trait as suggested in by @adamw in #351 (comment)). The same goes for RawBodyType.FileBody. But we need FileBody when converting to sttp and if I get this right, it expects it to be a org.scalajs.dom.File. So one idea would be to use a dom file abstraction like sttp does. What do you think?

Any feedback or help much appreciated.

build.sbt Outdated Show resolved Hide resolved
codec
.encode(test1)
.shouldBe(
// In Scala.js in general, a trailing .0 is omitted i.e. 1.0.toString == "1", instead of "1.0"
Copy link
Member

Choose a reason for hiding this comment

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

:D

@adamw
Copy link
Member

adamw commented Oct 27, 2020

Great work, thank you!

I had to disable 8 of 172 tests in coreJS because they only work on the JVM for various reasons (i.e. file access or using the compiler API). The remaining ones pass except for MatchTypeTest because of the different JS semantics wrt. primitive number types. I'm not sure what to do about that.

I don't think we'll be able to do much here, apart from documenting this fact in the scaladocs.

Similarly, a few tests in JSON libraries are disabled for JS because there are some missing things i.e. regarding Date.

Make sense. As long as they are still there in the jvm version, that's fine (and I saw that they are)

One thing I'm still unsure about is how to do file support for Scala.js. For now i've just disabled the File and Path codecs for JS (via a CodecExtensions trait as suggested in by @adamw in #351 (comment)). The same goes for RawBodyType.FileBody. But we need FileBody when converting to sttp and if I get this right, it expects it to be a org.scalajs.dom.File. So one idea would be to use a dom file abstraction like sttp does. What do you think?

I would do it exactly the same as in sttp client, copying the code mercilessly if needed :). I'm not an expert on Scala.JS, but I think the approach from sttp client is good and works, so there's no need to come up with something else. Will make maintenance easier.

The travis tests seem to fail with an OOM on travis, not sure why this could be. Scalajs builds fine for sttp, although it uses jdk11 and bionic, so maybe this makes some difference.

@adamw adamw mentioned this pull request Oct 27, 2020
4 tasks
@sbrunk sbrunk force-pushed the scalajs-support-new branch 8 times, most recently from 7e0b902 to b3dd18d Compare November 1, 2020 13:47
@sbrunk
Copy link
Contributor Author

sbrunk commented Nov 1, 2020

Thanks for your feedback @adamw!

I had a look at sttp's file abstraction and I think in Tapir it could even be a bit simpler using only type aliases because there are less operations on files. So this is basically how it looks now:

JVM:

object FileExtensions {
  type TapirFile = java.io.File
  type TapirPath = java.nio.file.Path
}

JS:

object FileExtensions {
  type TapirFile = org.scalajs.dom.File
  type TapirPath = org.scalajs.dom.File
}

Shared code always refers to TapirFile while code that needs to deal with files directly is put into platform specific extension traits.

Regarding the travis failures: I tried a few things like setting JAVA_OPTS in travis but I only managed to get rid of the OOM error by adding an .sbtopts. Of course that means it is also applied locally. Not sure if that's something you want.

There are still errors though. I'm looking into the org.scalajs.testing.adapter.JSEnvRPC$RunTerminatedException. Ironically I found a comment from you in the related issue about something similar in sttp: scala-js/scala-js#3174 (comment)

playServer and http4sServer are failing too. This also happens locally but if I run them alone, it works so it's a bit hard to debug.

@@ -0,0 +1,6 @@
package sttp.tapir

object FileExtensions {
Copy link
Member

@adamw adamw Nov 2, 2020

Choose a reason for hiding this comment

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

to consider: maybe this could be part of TapirExtensions, meaning that it would be automatically imported in the package object?

& we would need to extend TapirAliases with trait FileExtensions as well possible

@adamw
Copy link
Member

adamw commented Nov 2, 2020

I've tried running the test but I'm getting a:

Error: Cannot find module 'jsdom'

which probably shouldn't be too suprising as https://github.com/scala-js/scala-js-env-jsdom-nodejs says "Finally, make sure that jsdom 10.0.0 or later is avilable in your project.". Obviously I didn't do npm install jsdom beforehand.

I'm wondering how to best approach is. Would be best if the build was self-contained and didn't require having an node installation at hand (which brings various node version problems). But I guess this might not be possible. So maybe we could have some way of bringing in the dependency via an sbt dependency? Or maybe some build step that would do the npm install?

I'm not sure what the best practice in scala cross jvm/js projects is. In sttp, we download the chrome driver, but here that would be an overkill probably. Maybe phantomjs? Or is that not an option?

@sbrunk
Copy link
Contributor Author

sbrunk commented Nov 4, 2020

I tried phantomjs but it's not maintained anymore and it doesn't seem to support the current dom file API correctly. Furthermore, we'd also have to install phantomjs binaries as it's not done automatically by the plugin.

I found an automatic node installation in the frontend-maven-plugin. While it's a maven plugin, it seems the installer code is not maven specific and we might be able to just call it from sbt. But it could also be easier to put something directly into the build.

A real headless browser with scala-js-env-selenium/chromedriver like in sttp is of course more overhead, but if we need it for running JS tests with real requests for sttp-client anyway, it wouldn't matter that much. Does sttp run JS client tests against a JVM testserver? If so, do you think we should do the same here?

@adamw
Copy link
Member

adamw commented Nov 5, 2020

@sbrunk yes, sttp runs tests against a running jvm server, and it needs the headless browser environment as the JS backend is written using the fetch api which (as far as I'm aware) is only available in browsers (but I'm not an expert here, so I might be wrong :) ). So you are right that we'll need the same here anyway, as the next step is porting the client interpreter, which will need to be tested using the fetch backend.

So probably the best solution is to setup the tests the same as in sttp client :). The download-chrome-driver part can be probably moved to our sbt plugins, but for now we can just copy it and I'll extract it at a later time.

@sbrunk sbrunk force-pushed the scalajs-support-new branch 2 times, most recently from 8ba16cb to c25f4d5 Compare November 6, 2020 21:39
Borrow env setup and automatic chromedriver download from sttp.
@sbrunk sbrunk force-pushed the scalajs-support-new branch from c25f4d5 to c028246 Compare November 9, 2020 21:30
@adamw adamw merged commit cc039db into softwaremill:master Nov 12, 2020
@adamw
Copy link
Member

adamw commented Nov 12, 2020

First part done - thanks a lot! :)

Now - tapir-client-sttp?

@sbrunk
Copy link
Contributor Author

sbrunk commented Nov 12, 2020

Thanks for getting this over the finish line! Yes tapir-client-sttp will be next so we can actually use this ;-)

@sbrunk sbrunk deleted the scalajs-support-new branch November 12, 2020 16:22
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