Set console.error to console.log if unset

[japgolly]

Fixes #47

[sjrd]

Thanks for the PR.

Would it be possible to have a test for this in https://github.com/scala-js/scala-js-env-phantomjs/blob/master/phantomjs-env/src/test/scala/org/scalajs/jsenv/phantomjs/PhantomJSEnvTest.scala ? If not, why not?

Could you also include the issue number in the commit message:

Fix #47: Set console.error to console.log if unset

[gzm0]

I do not understand. The fix seems inconsistent with what is mentioned in scala-js/scala-js#1555 / ariya/phantomjs#13112. It says that PhantomJS crashes when console.error is called.

But the fix seems to set console.error when it is unset. Note that this is not what your existing workaround does. It simply sets console.error = console.log unconditionally.

It seems that this is a PhantomJS2 issue. So to properly test it, we'd have to start testing with PhantomJS2.

Feels like we have the following options:

• Just set console.error = console.log: Will break existing PhantomJS 1.x users.
• Detect PhantomJS 2, set console.error = console.log.
• Like before, + testing
• Leave the workaround in @japgolly's repos only (see Exceptions escape and crash the entire jsEnv #47).

[sjrd]

Apparently the CI is already running with PhantomJS 2.1.1: https://travis-ci.org/github/scala-js/scala-js-env-phantomjs/jobs/696416719#L119, which raises the question: why is it already working on master with PhantomJS 2, but not in the context of @japgolly's repos? :-s

[japgolly]

Would it be possible to have a test for this in https://github.com/scala-js/scala-js-env-phantomjs/blob/master/phantomjs-env/src/test/scala/org/scalajs/jsenv/phantomjs/PhantomJSEnvTest.scala ? If not, why not?

@sjrd Yes! But only a test that console.error('test') doesn't crash. I did genuinely try to minimise the real error I'm seeing where certain errors were triggering the custom onError code which would crash everything by calling console.error but I couldn't come up with a reproduction. Let me at least add a console.error test.

Feels like we have the following options:

@gzm0 Personally I don't care how we get there as long as console.error = console.log is called for PhantomJS2. The reason I went with the if (!console.error) guard is because it seems to be set to undefined for PhantomJS2. It's a logically consistent goal to just "fix up" console.error not being defined. I have no idea about PhantomJSv1 as it's older than the dinosaurs but I assume that in that env console.error will simply be defined and that should be ok? But of course if you need me to do it some other way I'm happy to accommodate :)

Leave the workaround in @japgolly's repos only (see #47).

I will just say I don't like this potential outcome. I wouldn't be surprised if I'm the guy that sends the most volume through scala-js-env-phantomjs, which is why I'm the only to come across this. But it is a genuine and general bug. If anyone else starts using scala-js-env-phantomjs heavily they'll likely come across this too.

[japgolly]

Guys any advice on testing this? Went with the following and it passes without the fix:

kit.start("console.error('test')").expectOut("test").closeRun()

[gzm0]

Well, if it fails without the fix, then its a good test :) Otherwise, it doesn't test what you fixed :)

[sjrd]

@japgolly What's the smallest reproduction of the issue that you managed to get so far? From there we might be able to suggest a minimal test case.

[sjrd]

I wouldn't be surprised if I'm the guy that sends the most volume through scala-js-env-phantomjs, which is why I'm the only to come across this.

TBH I don't understand why you're still using PhantomJS at all. It's been officially dead for years. The only reason we published scalajs-env-phantomjs for Scala.js 1.x is not to break existing workflows, but we've been recommending Node.js with jsdom for years.

Compare the download numbers for the jsdom environment:

with those of scalajs-env-phantomjs:

That's a 30x difference.

[japgolly]

Well, if it fails without the fix, then its a good test :) Otherwise, it doesn't test what you fixed :)

@gzm0 Think you misread me there. It passes without the fix. I wish it failed without the fix :D Bizarre.

@japgolly What's the smallest reproduction of the issue that you managed to get so far? From there we might be able to suggest a minimal test case.

None because it's too intertwined with all of my private code. :(

TBH I don't understand why you're still using PhantomJS at all. It's been officially dead for years.

Oh I know but two reasons:

1. for certain types of tests (esp CPU intensive load like property tests) it's orders of magnitude faster than node. Like 20 sec vs ~80sec which is huge.
2. legacy on my own part in that certain tests don't work when I've tried to switch to node. From memory, web workers might be a factor that node/jsdom doesn't support or something like that.