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.

Sign up for GitHub

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

Jump to bottom

Make JsEnv extensible #2144

Closed
nafg opened this issue Nov 25, 2022 · 4 comments
Closed

Make JsEnv extensible #2144

nafg opened this issue Nov 25, 2022 · 4 comments
Assignees
@lolgab
Milestone
0.10.10

Comments

@nafg
Copy link
Contributor

nafg commented Nov 25, 2022

In Mill, the JsEnv selection is based on a sealed trait with only 3 choices: NodeJs, JsDom, and Phantom (which is an obsolete project). But there are many cases when one would want to use a different strategy. As far as I can tell, currently the only way to accomplish that now is to override testTask and do everything from scratch, because all of the scalajs APIs are private.

One use case for this is described in scala-js/scala-js-env-jsdom-nodejs#44, where @sjrd suggests using https://github.com/exoego/scala-js-env-jsdom-nodejs. Hopefully that project will continue to be maintained, but in any case it's out of reach from Mill, as far as I can tell.

I think it would need a major redesign of the APIs. IIUC (mentioned in another issue) the reason for the current design is to enable one project to use multiple versions of Scala.js, which is a great feature that even SBT doesn't support. I don't know if it's possible to have both at the same time (user-supplied JsEnv and multiple scalajs versions in one project).

But I don't see any other way to use jsdom to test code that uses modules.
@sjrd
Copy link
Contributor

sjrd commented Nov 25, 2022

IIUC (mentioned in another issue) the reason for the current design is to enable one project to use multiple versions of Scala.js, which is a great feature that even SBT doesn't support. I don't know if it's possible to have both at the same time (user-supplied JsEnv and multiple scalajs versions in one project).

Since Scala.js 1.1.1, the JSEnv APIs come from separate artifacts with their own, backwards binary compatible versioning. So Mill could always use the latest version of scalajs-js-envs without compromising its ability to link with multiple versions of Scala.js.

@lefou
Copy link
Member

lefou commented Nov 25, 2022

I think some internal used code is private to avoid the maintenance costs of preserving binary compatibility, which we take seriously in Mill. But it's not set in stone, so if you come up with a good use case and a proper suggestion which API you need to change and how, we can change / improve it.

You need to be a bit more specific though, as your current description isn't actionable for me.

Also, I admit that Scala.js is not my field of expertise, so you better be verbose when describing the needed changes. I would prefer some draft pull request, so we can focus on concrete code and the impact of changes in terms of tests.

That being said, I'm very open to support more use cases and looking forward to it.

@lolgab
Copy link
Member

lolgab commented Nov 25, 2022

@sjrd I tried in the past to use the Scala.js API directly, but when changing version of Scala.js it was breaking the classpath. Maybe if we compiled with Scala.js 1.0.0 but then we couldn't use the newer features. If we compile against the latest versions then I remember that the older versions were breaking.
I think it is a similar problem to the one faced in mdoc scalameta/mdoc#629 which was solved by creating a copy of the Scala.js API and mapping it to the real Scala.js types in the separate classpath.
Since we need support every version at the same time the current approach is probably the best we can have.

@lolgab lolgab self-assigned this Nov 25, 2022
@sjrd
Copy link
Contributor

sjrd commented Nov 25, 2022

I'm not saying to statically link against the linker API of Scala.js.

But for https://github.com/scala-js/scala-js-js-envs in particular, you can always use the latest version. It is independent from everything else.

@lolgab lolgab mentioned this issue Nov 25, 2022
Merged
lefou pushed a commit that referenced this issue Nov 26, 2022
@lolgab
Support exoego fork of JsDom JsEnv (#2147)
7620024 
My first approach to solve #2144 was to create a `Custom` `JsEnvConfig`
which would take a `className` and then the implementation would
instantiate that class with java reflection. It worked fine but didn't
support parameters or builders used by the JsEnv to create itself.
Since there aren't many custom `JsEnv`s out there (the only one I know
is [exoego](https://github.com/exoego/scala-js-env-jsdom-nodejs)'s fork
of jsdom-nodejs, I'm proposing of supporting it officially in Mill.
This PR also changes the worker classpath to only download and load the
jsEnv dependencies that are needed. This skips downloading artifacts and
loading classes for jsEnvs that aren't used, like this new one for
people not using it, or the deprecated phantom jsEnv.
 
Testing this properly means installing `jsdom` which can only be done in
the root because of #1036 (to
my understanding). I tested it manually in scratch but I needed to have
a global `package.json`

Pull request: #2147
@lefou lefou closed this as completed May 2, 2023
@lefou lefou added this to the 0.10.10 milestone May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lolgab lolgab

Labels
None yet
Projects
None yet
Milestone
0.10.10
Development

No branches or pull requests
4 participants
@nafg @sjrd @lefou @lolgab