Fix JS feature detection so that it no longer crashes

[japgolly]

An example of this crashing in practice can be seen in https://github.com/japgolly/scalajs-react/blob/v2.0.0-RC2/downstream-tests/js/src/test/resources/polyfill.js where I had to add a fake importScripts as a workaround. A try/catch is necessary.

[armanbilge]

Thanks for this! Our test suite currently passes on Firefox and Chrome, is there a way we can test for this in the future?

[japgolly]

No worries! Yeah it should be reproducible by running the tests with jsEnv := new org.scalajs.jsenv.jsdomnodejs.JSDOMNodeJSEnv

[armanbilge]

Hmm, isn't that an artificial environment? Does this actually occur in the browser?

[japgolly]

When you say the tests pass on Firefox and Chrome, I presume you're not testing using Cats Effect on the web worker side? If you have the capacity it would be good to test that too because it's a different env that normal JS world.

[japgolly]

Hmm, isn't that an artificial environment?

Nah, it's a valid use-case to run Scala.JS from Node itself (eg. for AWS Lambdas).

[armanbilge]

I presume you're not testing using Cats Effect on the web worker side?

We are! I just added that #2165 actually.

[armanbilge]

Nah, it's a valid use-case to run Scala.JS from Node itself (eg. for AWS Lambdas).

Yes, well we also test in the Node.js environment for this reason :) Node.js, Chrome, and Firefox.

[japgolly]

Yes, well we also test in the Node.js environment for this reason

Oh you're already testing Node.js! Awesome! Hmmm, in that case maybe it's literally the jsdom interaction. But regardless, there are still two good reasons to adopt this (or a similar change):

1. Node with JSDOM is the standard way for unit testing the Scala.JS component of webapps
2. js.typeOf(js.Dynamic.global.xxx) is a direct copy of JS where polyfills do typeof(xxx) but it's an incorrect translation to Scala.JS. js.Dynamic.global.xxx will throw an exception in SJS where no exception would occur in JS.

[armanbilge]

Node with JSDOM is the standard way for unit testing the Scala.JS component of webapps

Sounds like maybe this should be added to the test environments.

[djspiewak]

I think you're probably right that this should be added to the test environments. The matrix grows ever wider…

[armanbilge]

I added the JSDOM environment and did a before-after with @japgolly's commit in #2190, unfortunately, it doesn't appear to fix the problem. TBH I'm not sure it's related to the execution context at all.

[vasilmkd]

How about !js.isUndefined(a)? Is that the same?

[armanbilge]

I think I tried this when I implemented the web worker polyfill and ran into problems, but maybe I did it wrong.

[japgolly]

No js.isUndefined wouldn't work because it only tests whether a strict/eager value is undefined or not, but the problem is an exception is being thrown when attempting to resolve that value. Personally I think it's silly behaviour but sjrd is very, very adamant in these things. It is what it is now so when testing for optional JS features we need to always be ready to catch a (Fatal) exception.

[vasilmkd]

Awesome, good to know. Thanks.

[armanbilge]

Ok, I fixed this in #2191, @japgolly would you mind giving it a try?

You were right, we did need a try/catch in the polyfill, but in a different place. The reason your importScripts hack works is because it was short-circuiting the branch that would otherwise throw the exception. AFAICT this only affects the JSDOMNodeJS environment.

[japgolly]

Ok, I fixed this in #2191, @japgolly would you mind giving it a try?

@armanbilge Yeah no worries, I'll give it a try later today and get back to you

[vasilmkd]

Is this PR superseded by #2191?

[vasilmkd]

Closing in favor of #2191.