Added JSDOMNodeJSEnv.Config.exposeGlobal

[viktor-podzigun]

Hi,

I've prepared this PR as a follow up on the discussion and suggested approach here: https://github.com/scalacenter/scalajs-bundler/issues/197

This will help to correctly identify environment (node.js) and solve this issue: https://github.com/scoverage/scalac-scoverage-plugin/issues/183

Looking forward to your feedback.

[sjrd]

If we're going there, I believe we should make it more general. global is not the only thing we might want to expose. Currently this config seems too specific to me, and I don't like specific stuff. If exposing certain global variables is necessary, then global should not be special-cased.

Maybe a withExposeGlobalVars(varNames: Set[String]) would be more appropriate. To expose global one would use withExposeGlobalVars(Set("global")).

[viktor-podzigun]

I see your point. How about exposeFullAccess boolean flag?

From the library user prospective its all he needs. Should not bother with specific details like variables and their potentially misspelled names. And we would have flexibility to expose more vars in the future (if new Node version adds more vars) if this flag is set. No changes needed from the user side in this case.

What do you think?

[sjrd]

What would "full access" mean/do?

[viktor-podzigun]

Without full access it is restricted browser-like environment, as it is now. With exposeFullAccess=true it becomes full Node.js env + browser staff.

For now it will expose global var like it is for Node.js-only env.

Do you think it is too generic now?

[sjrd]

But what does

full Node.js env + browser staff.

mean? Or in other words, how would you implement it? Iterating over all the global variables is not an option anymore, since ES2015 introduces global variables that cannot be listed. Also we wouldn't want to trample stuff already in the browser's global scope. Do exposing all global variables from Node.js is not an option.

[viktor-podzigun]

Sorry, maybe I was not clear about my idea.

I would implement it like proposed here, by exposing only global for now:

if (${config.exposeFullAccess}) {
  window["global"] = global;
}

Then to access Node-specific api like require("fs") on a user side one would do:

import scala.scalajs.js.Dynamic.{global => g}

g.global.require("fs")

or by defining a facade for it:

@js.native
@JSGlobal("global")
object NodeJs extends js.Object {
  def require(module: String): js.Any = js.native
}

NodeJs.require("fs")

I've checked it, it works for both JSDOMNodeJSEnv and NodeJSEnv.

[sjrd]

Right, so you're just proposing to change the name of the API. My point was, independently of the name of the API, only exposing global is too specific. We should allow the user to expose other global variables. This is especially important for ES2015 lets, consts and classes, because they won't be available through the global global variable. They are only available as themselves.

[gzm0]

I have had a look at the discussions that have led to this and it seems to me the overall approach has a fundamental problem: The burden on adjusting to JSEnvs is on scoverage and not on the JSEnvs themselves. So scoverage will have to do work over and over again for new JSEnvs. On the other hand, new envs have no chance of gracefully working with scoverage (e.g. the Selenium env will probably not work with this).

May I propose that we investigate a solution using scalaJSCom? A side channel that we use for testing instrumentation and we expect all JSEnvs to provide.

The downsides of a solution like this will be:

• scoverage will have to provide JVM artifacts even for SJS.
• core SJS will need hooks in its test adapter and JS interface.

The upside will be:

• Any JSEnv passing the conformance suite we provide will work with scoverage
• No JSEnv specific maintenance for scoverage
• No scoverage breakages because implementation details change.

[sjrd]

What kind of interface/hooks do you have in mind? Some sort of logging identified by a channel name, that the JVM side calling the JSEnv would be able to read? Even something pretty specific like that would be problematic for scoverage to use: they would have to hook into the TestAdapter infrastructure, somehow. Such an API will have to cross the boundaries of several abstractions, it seems.

[gzm0]

Probably something that sits below the TestAdapter infrastructure and provides channel multiplexing on a registration basis.

Something like this:

val send = Com.register("scoverage", msg => ...)

This would be available:

• To JS via an object.
• To the JVM via the sbt plugin (the interface there might be a bit challenging).

The JVM side might also need lifecycle information about the run. The TestAdapter would just be one user of this.

[gzm0]

I have been thinking some more and it's not that simple, since the runners are instrumented by the TestAdapter. It seems that we can meet the requirements by having a JS -> JVM RPC interface only.

So something like:

JS:

val x: Future[String] = SJSRpc.call("scoverage", args...)

JVM (sbt)

scalajsRPCHandlers += "scoverage" -> (args => Future(...))

The TestAdapter would not use this though.

[viktor-podzigun]

Right, so you're just proposing to change the name of the API. My point was, independently of the name of the API, only exposing global is too specific. We should allow the user to expose other global variables. This is especially important for ES2015 lets, consts and classes, because they won't be available through the global global variable. They are only available as themselves.

Ok, got it. Then maybe indeed we would be more flexible with the api you've proposed: withExposeGlobalVars(Set("global")) I'm fine with it.

[sjrd]

The TestAdapter would not use this though.

That probably defeats the purpose of that hook for scoverage, which really wants the instrumentation when running test.

[gzm0]

That probably defeats the purpose of that hook for scoverage, which really wants the instrumentation when running test.

Ah, sorry. That apparently wasn't clear. I meant the TestAdapter would provide this rather than use it itself. So it would definitely be available when using the TestAdapter, maybe even only when using the test adapter.

@viktor-podzigun how would such an approach work for you? (tl;dr you get a magic RPC channel where you can call stuff in the sbt's JVM from JS).

[viktor-podzigun]

I think it will slow down the execution, since it will need to go through the bridge on every instrumented statement.

As for now I would suggest to go with the existing approach there, which is proven to work well, and we may re-consider it again when we have more jsEnvs.

What is more important for me is to make it work with the scalajs-bundler as well, to re-use JSDOMNodeJSEnv there and remove local copy as was suggested by @sjrd.

[gzm0]

I think it will slow down the execution, since it will need to go through the bridge on every instrumented statement.

Fair enough. But isn't there some sort of batching anyways? (same way BufferedOutputStream also batches I/O). There is at least one round-trip per test anyways (due to sbt's synchronous interface), so batching coverage information per test would unlikely cause a significant slowdown.

when we have more jsEnvs.

Who is "we" in this statement? I think one problem with the current approach is precisely that JSEnv is not open for extension anymore when using scoverage.

I'm not very happy with the current approach because the customization we introduce seems very inconsistent with other things we could allow to customize (n.b. the page that is loaded).

You also mention compatibility with scalajs-bundler. From the issues you linked I couldn't find more details about what the problem with scalajs-bundler specifically is. Could you elaborate on that?

[viktor-podzigun]

Note: I'm not an scoverage dev, rather a user. Batching/buffering will definitely help, but build may consume more memory then, so the right trade off should be found.

On the other hand, Node.js is probably the most used env, which is already works, the problem is only when using it with jsdom.

So, my suggestion is to make it first work and then optimise for other env. The approach suggested in this PR is independent and useful to have for other use cases (when building/testing app for Electron, where UI has full access to the Node.js env, for example).

Here is the link to the discussion: https://github.com/scalacenter/scalajs-bundler/issues/197#issuecomment-476682338

[viktor-podzigun]

@sjrd I've updated the PR.

When updating it I was thinking, maybe it would be more flexible for the future if we would have possibility to define custom name for the exposed var, like withExposeGlobalVars(Map("yourCustomGlobalName" -> "global")) instead of just withExposeGlobalVars(Set("global")) for the api?

What do you think?

[viktor-podzigun]

Closing in favor of https://github.com/scoverage/scalac-scoverage-plugin/pull/456