Fixed #183: JSDOMNodeJSEnv is handled incorrectly (Scalajs support)

[viktor-podzigun]

Fixes #183

UPDATED! Original PR description:

In this PR made attempt to correctly identify Node.js environments (with or without jsdom).

In order to detect Node.js environment I'm using the global object (approach, suggested here):

if (js.typeOf(g.global) == "object" && js.typeOf(g.global.require) == "function") {
    // Node.js environment detected
    NodeFile
}

It doesn't break existing logic and it is a step forward to fixing #183.

There should be also a corresponding change applied for JSDOMNodeJSEnv class in scala-js/scalajs-bundler to export global object, either by extending it with a setting to do so, or by creating separate implementation (see discussion here).

[viktor-podzigun]

Hi @gslowikowski, could you, please, take a look to this PR when you will have some time?

Since there is a strong demand to make scoverage work for UI testing in scala-js using Node.js/jsdom env, it would be really great if we could make it work at least from scoverage side.

Thanks!

[nimaeskandary]

Any updates on this?

[gslowikowski]

Hi

I've created simple project with jsdom env https://github.com/gslowikowski/scoverage-scalajs-node-jsdom-example, but JS module tests fail when building with the version of scoverage plugin from this pull request.

Maybe I'm doing something wrong. @viktor-podzigun can you help me to verify your PR?

[viktor-podzigun]

Hi @gslowikowski,
thank you for taking care of it.

I've made PR to your test project with some changes, please, see my comments in build.sbt file:
https://github.com/gslowikowski/scoverage-scalajs-node-jsdom-example/pull/1/files

I think we would need to make one more PR to make it work with JSDOMNodeJSEnv, as the one I've made for scalajs-bundler

[ckipp01]

@viktor-podzigun I know this has been stale for quite some time, but is this still relevant? If so, are you able to rebase?

[viktor-podzigun]

@ckipp01 I've re-based it already.

And, yes, I think, its still relevant. As you can see, the PhantomFile js environment is assumed by the window global var, which is not always correct. As described in the original linked issue #183 , the node.js + jsdom env. would resolve to phantom!

[viktor-podzigun]

@ckipp01 ok, re-based it one more time on the latest main branch

[ckipp01]

@ckipp01 I've re-based it already.

And, yes, I think, its still relevant. As you can see, the PhantomFile js environment is assumed by the window global var, which is not always correct. As described in the original linked issue #183 , the node.js + jsdom env. would resolve to phantom!

gslowikowski/scoverage-scalajs-node-jsdom-example#1 (comment)

From this comment though, which has already been a couple years ago, it seems that this is sort of no longer a thing?

[viktor-podzigun]

@ckipp01 maybe Phantom is no longer a thing, but that's exactly why we should make it work with node.js in the first place :) and that is this change proposed here is all about. The thing is, that when we install jsdom in node.js env. it is detected as Phantom atm.

[ckipp01]

@ckipp01 maybe Phantom is no longer a thing, but that's exactly why we should make it work with node.js in the first place :) and that is this change proposed here is all about. The thing is, that when we install jsdom in node.js env. it is detected as Phantom atm.

ahhhh, totally misunderstood that. Thanks for clarifying.

[ckipp01]

Thanks for the pr @viktor-podzigun and sorry it's stood still for so long. LGTM