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

Remove Scala.js shims for JDK classes #437

Closed
armanbilge opened this issue Oct 16, 2021 · 2 comments · Fixed by #441
Closed

Remove Scala.js shims for JDK classes #437

armanbilge opened this issue Oct 16, 2021 · 2 comments · Fixed by #441
Milestone

Comments

@armanbilge
Copy link
Contributor

armanbilge commented Oct 16, 2021

Specifically: https://github.com/scalameta/munit/tree/main/munit/js/src/main/scala/java

I know this will be an annoying change to make, but it would be great if this could land in 1.0 as well. I understand these shims are important for implementing munit features, but they are also potential landmines.

Here are two ways it can go wrong (and has for me):

  1. I write a Scala.js library accidentally using File or Path or one of the other classes shimmed here. I write an munit suite to test it, and voila it all works (b/c munit has kindly put these JDK shims on the test classpath). So I publish my library. Then, a downstream depends on my library and tries to create a Scala.js app ... but they can't, because they get linking errors for Path, File, etc. Having JDK shims on the test classpath makes it nigh impossible for libraries to catch these sorts of mistakes.
  2. If some other well-meaning library or application has also shimmed these JDK classes, then there would be a classpath conflict, in which case I'm actually not sure what happens. But probably not good.

Off the top of my head, there's a couple ways to avoid this (not mutually exclusive):

  1. Move these shims out of the java package into the munit package and in your JVM sources create type aliases to point e.g. munit.internal.Path to the JDK implementations.
  2. Create wrapper classes in the munit package, that delegate to JDK on the JVM and Node.js on JS.

Thanks for your attention to this issue.

@olafurpg
Copy link
Member

Thank you for reporting! I agree it's problematic to provide JDK shims. I'm OK with either approach you propose. I estimate it's low-effort because most of the file I/O happens in this file here (it's only a few method calls) https://github.com/scalameta/munit/blob/ca8845aaf2aee98111de337a9b225eb4c8849850/munit/shared/src/main/scala/munit/internal/console/Lines.scala

@armanbilge you would be interested in contributing a PR? It would be nice to include this in 1.0 since it's arguably a breaking change to remove the JDK shims.

@olafurpg olafurpg added this to the MUnit v1.0 milestone Oct 17, 2021
@armanbilge
Copy link
Contributor Author

Sure, why not, if it's as easy as you say. I hope I don't regret this! 😝

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 a pull request may close this issue.

2 participants