-
Notifications
You must be signed in to change notification settings - Fork 791
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
Cross-build http4s for Scala.js #4938
Conversation
This reverts commit 7fa1fcb.
All tests are passing!! Besides the occasional hiccup with Now, I seek guidance on a couple remaining issues so we can hopefully get this merged :) Some TODO's I scattered in the code:
A couple other more minor things:
Thanks in advance for your feedback/reviews!! |
After chatting with @ChristopherDavenport on Discord, my understanding is the thinking is to prioritize support for browser-based users without locking out other JS platforms. So, I'm going to use the Web Crypto APIs to plug the crypto hole for now, and we can hopefully take advantage of its experimental presence in Node.js and/or provide a fallback implementation at a later point. |
@rossabaker I think I've addressed most of your review comments, let me know if I've missed anything. I think I restored all of the headers and I did my best undoing at least some of the build formating. See my comments above re the Node.js/python server scripts and the crypto library. We'll be getting fs2 3.1 very soon which will let me make a final pass of cleanups, it doesn't necessarily have to be this PR. |
project/Http4sPlugin.scala
Outdated
attributedSources.contains(baseDirectory.value.toPath.relativize(file.toPath).toString) | ||
} | ||
|
||
val attributedSources = Set( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether we even need this hack if sbt-header is leaving the attributions alone now? That would be lovely, because you just experienced firsthand how fragile this is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't (assuming you are happy with the dual-headers), I can remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, what you did looks great. People have tried to tear this project down for stealing work over the years, but I don't think many projects go to our lengths to credit their sources. Thanks for persevering on this to keep that tradition alive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had no idea about that history, really sad to hear because it's readily apparent you've gone above and beyond here.
I'd say privatize whatever you see fit in the JS package, and then we'll land this and cut a milestone. |
Hooray!! I'll take a careful pass through the JS package and let you know when it's ready.
Do you still want to wait for a proper scala 3 artifact for |
Actually, I have an idea: Edit: on the other hand, that would still be adding a dom dependency to http4s core ... which is just weird. This is a tricky one. |
I don't want to keep this PR open longer than necessary. I would exclude the DOM packages from publishing on Scala 3 for this round, and we can reintroduce later. I don't think it's terrible if that DOM dependency in core is JS only, if it's necessary to make the core run on JS? Maybe I'm misunderstanding. |
|
||
var iterations: Double = js.native | ||
|
||
var hash: HashAlgorithmIdentifier = js.native |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting a ton of warnings like this. Maybe we're stuck with them:
[warn] /Users/ross.baker/src/http4s/core/js/src/main/scala/org/http4s/js/crypto/Crypto.scala:863:42: dead code following this construct
[warn] var hash: HashAlgorithmIdentifier = js.native
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 2.13 is better with this, because I tried @nowarn
ing but then 2.13 started complaining there was nothing to @nowarn
. This could probably be solved with a magic incantation of sbt settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not terribly worried about it, but there's enough scala.JS only code, it would be a nice mopup later so we don't miss bugs there by tolerating warnings.
Me neither, let's close this out. So it's okay if we do a breaking bump for
Yes, just talking JS here. A DOM (browser) dependency for core is weird because core also runs on non-browser environments such as Node.js. So it should be agnostic. |
I took the liberty of drafting a changelog here. My JavaScript and frontend knowledge is about 15 years out of date, so please don't hesitate to correct anything silly that I said.
For sure. 1.0 is very much under development.
And it gets pulled into http4s-core because it provides the JS crypto, otherwise it would be in http4s-dom-core? Seems like maybe that scala-js-dom is missing some modularity? |
I took a brief look, looks good! 😄
Then FWIW, there's no harm to release Scala 3 artifacts using
I'd say it's a Scala.js problem as a whole, because they are trying to use a single artifact suffix I get the feeling your itching to release, say tomorrow? I'll wrap up everything that's left tonight. |
"Never" is a strong word, and I'd rather not normalize
I'm not sure the solution to that problem is copying massive amounts of code. What is the real downside to having all this DOM support in an http4s-core for Node.js? I don't have a good grasp of what the optimizing compiler does for us here.
Would love to merge at least. Release would be great. It would be a bit cleaner if we had an fs2 milestone to keep like qualifiers with our dependencies. But not strictly necessary, I guess. /cc @mpilquist |
I can say so with certainty because of scala-js/scala-js-dom#451. Basically, the
The risk is that that library evolves in ways that strongly diverge from our needs. For example, attempting to use their crypto facade on a non-DOM environment i.e. Node.js is currently throwing a fatal exception, because of a package object initializer that assumes DOM stuff. This is why I copy-pasted the code in the first place. Theoretically, we can try and patch these (and pray that they accept our PRs) but at the end of the day our goals are just not well-aligned. After crossing many many projects, I do think we need a cross-platform crypto library that supports browsers, Node.js, and maybe even JVM. Http4s, Skunk, and scodec all have this common need.
Yes, let's merge today please. I would also prefer to wait for fs2 to release, but I think that's still be going to be a little while. Last night and one more time this morning I took a pass through the JS cryptography facade. Unfortunately, I don't think we can make much of it private without being surgical. I just pushed a fix for the build but otherwise I'm 👍 to merge. Thanks! |
Hoping for fs2 3.1.0 release tonight |
Okay. I can start a release train this afternoon then, since this will come third. Before that happens, someone needs to explain the security alerts to me like I'm a dumb backend developer: #5031 |
Cross-build http4s for Scala.js
Cross-build http4s for Scala.js
Cross-build http4s for Scala.js
tl;dr try it out with
This PR introduces cross-builds to Scala.js for several of the core http4s modules per #956:
core
laws
testing
tests
server
client
ember-core
ember-server
ember-client
node-serverless
new!dom-core
new!dom-fetch-client
new!dom-service-worker
new!dsl
boopickle
jawn
circe
It's still a bit rough around the edges, but very nearly there I think (tests are passing on my machine). The main gaps for Scala.js are file-
and crypto-related stuff, not sure how important these are.At this time, there is no server or client implementation for Scala.js but I think that is best done in follow-up work :)I've implemented a fetch-based client for browsers, and ember ison the way viahere! typelevel/fs2#2453.Admin stuff: this is my first PR to this repo so that means every commit I push will have to be manually approved for CI, which makes it tricky to work independently. Could I possibly get manually whitelisted, or alternatively if there's another small PR I can get merged right away (e.g., fix typo etc.) that would also get me on the whitelist. Thanks!