-
Notifications
You must be signed in to change notification settings - Fork 45
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
update scalajs-dom to 2.0.0 #377
update scalajs-dom to 2.0.0 #377
Conversation
CastConversion(stdNames.lib + Name("ChannelSplitterNode"), QualifiedName("org.scalajs.dom.ChannelSplitterNode")), | ||
CastConversion(stdNames.lib + Name("CharacterData"), QualifiedName("org.scalajs.dom.CharacterData")), | ||
CastConversion(stdNames.lib + Name("ClientQueryOptions"), QualifiedName("org.scalajs.dom.ClientQueryOptions")), | ||
CastConversion(stdNames.lib + Name("DOMRect"), QualifiedName("org.scalajs.dom.DOMRect")), |
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.
This file is my biggest concern actually, What you're looking at is a table of named DOM types in typescript and in scalajs-dom used for translation. I generated this at some point.
I'd like to confirm that all the types referenced on the right hand side, for instance org.scalajs.dom.DOMRect
actually exist. Do you have any script foo to do that? otherwise I'll get to it eventually :)
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.
Maybe the auto-generated API reports can be of some use for this? https://github.com/scala-js/scala-js-dom/blob/master/api-reports/2_13.txt
Thank you, most of the work is done now! :) Gotta love the scala churn, it never stops. This change is entangled with the scalajs-react major version, which means this PR is entangled with #326 . I rebased that on top of your PR, and all the tests seem to pass, which is good. That PR is setup to publish snapshots, so we'll publish snapshots of the two PRs combined until both slinky and scalajs-react have stable releases. Hopefully that will be soon. diffsThe diff is huge because this project uses snapshot tests - all of those files under Likewise the huge files underneath |
Snapshot published as |
Thank you both so much for taking this on, much much appreciated and sincere apologies for the churn :( let me know if there's something I can help with. |
Ok, so I used this regex to extract the scala-js-dom types: QualifiedName\("org\.scalajs\.dom\.(.*)"\) which I made into a list of urls which I checked the HTTP status of: while read LINE; do
curl -o /dev/null --silent --head --write-out "%{http_code} $LINE\n" "$LINE"
done giving me this list: https://gist.githubusercontent.com/armanbilge/0637566f9babf52986e9dea6dd67b630/raw/30b4f9eb557fbae5393827423f6c2eedb7b38a63/gistfile1.txt. There were some 404s in there, but many of those were files moved to scala-2/3 split sources: https://gist.githubusercontent.com/armanbilge/0637566f9babf52986e9dea6dd67b630/raw/943426f38c7249da596d5d261a43467f3141bcca/gistfile2.txt So in the end, we are left with:
I think these are type aliases or various other things, so I'm going to track them down and make suggestions on the PR if any need fixing. |
...s/src/main/scala/org/scalablytyped/converter/internal/scalajs/flavours/ScalaJsDomNames.scala
Outdated
Show resolved
Hide resolved
...s/src/main/scala/org/scalablytyped/converter/internal/scalajs/flavours/ScalaJsDomNames.scala
Outdated
Show resolved
Hide resolved
...s/src/main/scala/org/scalablytyped/converter/internal/scalajs/flavours/ScalaJsDomNames.scala
Outdated
Show resolved
Hide resolved
...s/src/main/scala/org/scalablytyped/converter/internal/scalajs/flavours/ScalaJsDomNames.scala
Outdated
Show resolved
Hide resolved
Thank you @armanbilge , exactly what I had in mind we would check :) |
Would it be straightforward to release a snapshot depending on scalajs-react 2.0.0-RC5 ? My deepest thanks to everyone involved. |
Hmm. I realize we would need a snapshot that also integrates this PR in order to be able to migrate to scalajs-dom 2. |
Thank you for the quick response!
I'm doing
We have |
Thanks for reporting, that's because of the changes in scala-js/scala-js-dom#593. It looks like the generator in ST will have to be adjusted to accommodate for that. I am surprised that setting Edit: ah, it's probably to due with the flavour setting. The ST docs say that
|
I wrote a test to enforce that all right hand sides in that conversion tables exist, and that the number of type parameters matches. Then I fixed the few things which showed up. The biggest surprise was probably that For reference, here is a list of types in scala-js-dom which ST will not translate to currently. I don't intend to add any of those to the conversion list now, but anyone can certainly feel free: somewhat long list
I also bumped #326 to use 2.0.0-M5, a snapshot if hopefully on the way |
Yes, I think that was done in scala-js/scala-js-dom#569. Hope it didn't cause too much trouble 😅 Quite a list there! |
No, not a problem at all @armanbilge :) snapshot published with version |
Thanks for the extensive review and comments. I might have bitten off more than I can chew with this PR, as my SJS and ST experience are limited, and this PR grew a lot. I see the build is still red, but I'm not sure what the exact reason for it is and what still needs to be done for this PR to be merged 😅 |
@hugo-vrijswijk as previously mentioned this PR is entangled with #326 which IIUC means they need to be merged at the same time (this one cannot standalone). Given that #326 is rebased against this, the fact that it is green there is a good sign :) |
Awesome, our library compiles with this version. Thank you very much! |
scala.js react 2.0.0 final has been released with not much changes https://github.com/japgolly/scalajs-react/blob/master/doc/changelog/2.0.0-RCs.md#changes-in-between-rc5-and-200-final |
Now we only need a stable version of slinky :) |
Co-authored-by: Arman Bilge <armanbilge@gmail.com>
…erify number of type parameters.
Ok, tired of being blocked. This is going in anyway, and I'll release when possible. with a slinky snapshot version if necessary |
Update scalajs-dom to 2.0.0. This version is not binary compatible with 1.2.0. I was running into an issue where some of my other dependencies had upgraded, but ST hadn't, resulting in an eviction error from sbt.
I've mostly followed the migration guide:
dom.raw
,dom.experimental
,dom.crypto
to replace withdom
importsTwo pre-release versions are used to be able to use scalajs-dom 2 on tests projects:
I realize the changelog for this is massive. But because of the changed imports there's not really a way around it. When someone reviews this, perhaps there's a way to filter out certain diffs (like anything that starts with
scalajs.dom.
?