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

Support exoego fork of JsDom JsEnv #2147

Merged
merged 1 commit into from
Nov 26, 2022
Merged

Conversation

lolgab
Copy link
Member

@lolgab lolgab commented Nov 25, 2022

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 JsEnvs out there (the only one I know is exoego'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 in manually in scratch but I needed to have a global package.json

@nafg
Copy link
Contributor

nafg commented Nov 25, 2022

Thank so much!

I think @sjrd mentioned that there's a jsenv for using selenium. And in some ticket it was implied that users writing their own jsenv was a normal use case.

@lolgab
Copy link
Member Author

lolgab commented Nov 25, 2022

Thank so much!

I think @sjrd mentioned that there's a jsenv for using selenium. And in some ticket it was implied that users writing their own jsenv was a normal use case.

Supporting custom jsEnvs is harder and the only solution I have at the moment is clunky, which makes me prefer to support them in the core repository.
We can re-discuss this again when we are in bin-compat breaking window and we dropped support for Scala.js 0.6.
Also if something is supported doesn't mean that there are actual users writing their own private JsEnvs implementations and are also using Mill. When these users appear, we can talk and understand what to do. Adding a partial feature that uses reflection and is ugly for no-users is a no for me.

@lolgab
Copy link
Member Author

lolgab commented Nov 25, 2022

I've just noticed the scala-js-env-selenium which I can add to this PR as well.

EDIT: The selenium jsEnv has multiple parameters and it's not as easy as I thought to support. So I will not add it to this PR.

@lolgab lolgab marked this pull request as ready for review November 25, 2022 17:57
@lolgab lolgab requested a review from lefou November 25, 2022 17:57
@@ -50,6 +50,21 @@ trait ScalaJSModule extends scalalib.ScalaModule { outer =>
)
}

def scalaJSJsEnvIvyDeps: Target[Agg[Dep]] = T {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm very happy with this change which will speed up downloading dependencies in Scala.js builds, since now Mill downloads only the dependency you need instead of downloading all of them :)

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Nice one. I like that change too.

Would it be possible to add an additional Custom config, which requires the user to define the dependency as well as the executable and the parameters? That way, we don't need to change whenever a user wants to play with a new JsEnv. It might be not as safe, tested and convenient as the supported types, but also don't stand in the way.

@lolgab
Copy link
Member Author

lolgab commented Nov 26, 2022

Nice one. I like that change too.

Would it be possible to add an additional Custom config, which requires the user to define the dependency as well as the executable and the parameters? That way, we don't need to change whenever a user wants to play with a new JsEnv. It might be not as safe, tested and convenient as the supported types, but also don't stand in the way.

I tried but the problem is that executable and args is not something generic that comes from JsEnv but something specific to the node jsenv..
If you see the selenium one (which is the only official one we don't support yet), it has a completely different set of parameters.

@lefou
Copy link
Member

lefou commented Nov 26, 2022

Nice one. I like that change too.
Would it be possible to add an additional Custom config, which requires the user to define the dependency as well as the executable and the parameters? That way, we don't need to change whenever a user wants to play with a new JsEnv. It might be not as safe, tested and convenient as the supported types, but also don't stand in the way.

I tried but the problem is that executable and args is not something generic that comes from JsEnv but something specific to the node jsenv.. If you see the selenium one (which is the only official one we don't support yet), it has a completely different set of parameters.

This means we need to provide some factory function which is actually creating the JsEnv instance. But as this function needs access to the implementation via the current classpath, we can't provide it in the build.sc. Is this understanding correct?

Here is an idea. Maybe, we can delegate to some factory on the freshly build classpath. E.g. the user implements an factory (() => JsEnv or (someContextProvidedByMill) => JsEnv ) which is returning a customized instance of JsEnv and we just run it to get the instance. As this factory is in the sources of the (test) module, the compile classpath already contains the specific JsEnv.

In Mill, we then need the correct dependency and the class name of the factory.

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'll merge as is. Thank you!

Potential generic support for other JsEnvs can be discussed and implemented independently.

@lefou lefou merged commit 7620024 into com-lihaoyi:main Nov 26, 2022
@lefou lefou added this to the 0.10.10 milestone Nov 26, 2022
@lolgab lolgab deleted the nodejs-jsdom-env branch November 26, 2022 15:56
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.

3 participants