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 setup of __ScalaJSEnv.javaSystemProperties from the sbt plugin #3295

Closed
gzm0 opened this Issue Mar 11, 2018 · 6 comments

Comments

Projects
None yet
2 participants
@gzm0
Contributor

gzm0 commented Mar 11, 2018

Was: Proposal: Make __ScalaJSEnv.javaSystemProperties the responsibility of the JSEnv (changed after #3295 (comment))

Currently we fully manage Java System Property injection in the sbt plugin. As a result, we need to understand details about the underlying module structure.

Further, note that many JSEnvs further inject information into __ScalaJSEnv (source mapper, exit function). I propose that we make __ScalaJSEnv generation and injection fully the responsibility of a JSEnv by adding javaSystemProperties: Map[String, String] (additionalJavaSystemProperties?) to the RunConfig (after #3255).

As a result

  • __ScalaJSEnv becomes the sole responsibility of the JSEnv and hence can be better isolated.
  • The sbt plugin does not need to know about module loading details anymore.
  • JSEnvs can easily add information about themselves to javaSystemProperties if they so desire.

This, together with #3294 would allow us to remove jsExecutionFiles and call it jsInput. As a result, our core supporting infrastructure would be completely agnostic of the JS module type.

@gzm0 gzm0 added the enhancement label Mar 11, 2018

@gzm0 gzm0 added this to the v1.0.0-M4 milestone Mar 11, 2018

@gzm0

This comment has been minimized.

Contributor

gzm0 commented Mar 11, 2018

@sjrd WDYT?

@sjrd

This comment has been minimized.

Member

sjrd commented Mar 12, 2018

I don't have a strong opinion about this. I've always thought the Java system property support was a necessary evil, which did not fit very well anyway. If we move this "hack" from the sbt plugin to the JS env, IMO, it's still a hack. If you believe it is better isolated in the JSEnvs, I'm fine with it. And if that solves an issue with dealing with modules, why not?

@sjrd

This comment has been minimized.

Member

sjrd commented Mar 12, 2018

The sbt plugin does not need to know about module loading details anymore.

See my comment at #3294 (comment). Does the sbt plugin actually need to know about module loading details to fill in the Java system properties? It doesn't look like it from the current snippet in the sbt plugin. Or am I missing something?

@gzm0

This comment has been minimized.

Contributor

gzm0 commented Mar 12, 2018

It doesn't look like it from the current snippet in the sbt plugin. Or am I missing something?

It currently doesn't. But if we start loading things like real modules, the name __ScalaJSEnv might simply not reference the right value.

The other option is to not do this anymore (in the public plugin). Why did we add this to the public plugin anyways?

@sjrd

This comment has been minimized.

Member

sjrd commented Mar 12, 2018

It currently doesn't. But if we start loading things like real modules, the name __ScalaJSEnv might simply not reference the right value.

I see. I guess then for the test adapter/bridge flag, we would somehow force it to be under a very weird name as a property of the global object, so that modules don't matter. Something we can't really do with __ScalaJSEnv because it is already kind-of specified that it works by lexical scope.

Why did we add this to the public plugin anyways?

Initially, because it was the only means of parameterizing JUnit test suites, and hence we needed it for our own build. Back then, we tended to add to the public plugin everything we needed internally.

That strategy might not apply anymore, in which case we could indeed remove that support from the public sbt plugin, and instead provide the snippet in a How-to section in the documentation.

@gzm0

This comment has been minimized.

Contributor

gzm0 commented Mar 13, 2018

That strategy might not apply anymore, in which case we could indeed remove that support from the public sbt plugin, and instead provide the snippet in a How-to section in the documentation.

Yes, I think we should do this.

@sjrd sjrd changed the title from Proposal: Make __ScalaJSEnv.javaSystemProperties the responsibility of the JSEnv to Remove setup of __ScalaJSEnv.javaSystemProperties from the sbt plugin Mar 16, 2018

@sjrd sjrd self-assigned this Mar 16, 2018

sjrd added a commit to sjrd/scala-js that referenced this issue Mar 16, 2018

Fix scala-js#3295: Do not set __ScalaJSEnv.javaSystemProperties in th…
…e sbt plugin.

Instead, we set it up in our own build.

That feature was initially included in the sbt plugin because our
build needed it, but it has always been a bit hacky. In particular,
it requires the sbt plugin to know too much about how JS files are
executed, notably whether they are scripts or modules. Moreover,
that feature was the only reason left that the sbt plugin needed
to know about `__ScalaJSEnv` at all.

@sjrd sjrd closed this in 05aac3f Mar 17, 2018

sjrd added a commit that referenced this issue Mar 17, 2018

Merge pull request #3302 from sjrd/no-setup-java-sys-props
Fix #3295: Do not set __ScalaJSEnv.javaSystemProperties in the sbt plugin.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment