scala-js / scala-js-env-jsdom-nodejs

Node.js with jsdom environment for Scala.js
BSD 3-Clause "New" or "Revised" License
8 stars 10 forks source link

The jsdom Node.js env intercepts a JS exception #42

Closed japgolly closed 4 years ago

japgolly commented 4 years ago

A bit of background first: React allows users to define a componentDidCatch lifecycle hook that is invoked when a child component throws an exception. Users can use this to maybe display the error message on the screen and prevent the error bubbling up further.

I have a test in scalajs-react that covers this behaviour. Since upgrading to Scala.JS 1.0, the error isn't caught by React. Instead it acts as if I typed in ??? rather than render.

I've confirmed that if I run the code in the browser, React catches and handles the error gracefully, rendering exactly what my test case expects. Therefore I think it's something about either the test runner or the node environment.

(Reproduction can be provided soon / on demand)

japgolly commented 4 years ago

I've also tried this with both jsdom 9.12.0, 15.something and 16.latest. Same results across the board. This is probably the same as https://github.com/scala-js/scala-js/issues/3458

sjrd commented 4 years ago

A reproduction would be awesome!

Does this happen only with the jsdom environment, or can it be reproduce with another environment with DOM support like Selenium?

Harder question: can it be reproduced in a non-DOM environment?

japgolly commented 4 years ago

An isolated, minimised reproduction is basically too hard because it's in the React internals that the error is thrown and caught. But I can provide a reproduction using scalajs-react:

  1. Checkout https://github.com/japgolly/scalajs-react/tree/tmp/4029
  2. sbt
  3. test/testOnly -- japgolly.scalajs.react.core.ScalaComponentPTest.lifecycle1

Error occurs at https://github.com/japgolly/scalajs-react/blob/tmp/4029/test/src/test/scala/japgolly/scalajs/react/core/ScalaComponentTest.scala#L171

Regarding the different envs:

japgolly commented 4 years ago

Also try ... catch doesn't catch the error either :fearful:

sjrd commented 4 years ago

When I try your repro, I hit the FileSystemLoopException exception from the other report. What did you change to get to running the test to reproduce the above?

japgolly commented 4 years ago

Try with JDK 11

sjrd commented 4 years ago

Try with JDK 11

Thanks. Now I can reproduce this.

Just FTR, one's got to love a cs-based installation of Scala. Even on Windows, trying this was as easy as

$ cs launch --jvm 11 sbt-launcher
sjrd commented 4 years ago

FWIW, I can make the scalajs-react test pass if I comment out/delete the following lines: https://github.com/scala-js/scala-js-env-jsdom-nodejs/blob/e2adf2b9c9d6e5b1f156e4af35b4e669730187a0/jsdom-nodejs-env/src/main/scala/org/scalajs/jsenv/jsdomnodejs/JSDOMNodeJSEnv.scala#L88-L102 However, if I do that, all the tests that make sure that a truly uncaught exception causes the invocation to fail start to fail (i.e., an uncaught exception does not fail the run, whereas it should).

sjrd commented 4 years ago

I can also make the test pass if I keep the setup, but I empty the content of the function (error) { ... }. So my current strategy is to, in that function, somehow detect that we're being invoked from React's hack, to counter-hack it and just return, without displaying the error and exit the process.

gzm0 commented 4 years ago

What is cs?

sjrd commented 4 years ago

What is cs?

The coursier CLI, that we're hoping to make the default way to install Scala.

japgolly commented 4 years ago

It's curious that the Selenium jsenv doesn't have this issue. Presumably it's also got some code for catching errant exceptions yet doesn't catch this one that React does.

sjrd commented 4 years ago

Selenium fails a run by default if there is an uncaught exception. We don't have to convince it to do so.

I don't understand why React's code ends up as a "jsdomError". AFACIT it should trigger an "error", not "jsdomError". This is why things go wrong in jsdom I think.

sjrd commented 4 years ago

Hum ... rereading https://github.com/jsdom/jsdom#virtual-consoles, apparently "jsdomError" is intentionally triggered when there is an uncaught exception not handled by a window onerror handler. They say:

This is similar to how error messages often show up in web browser consoles, even if they are not initiated by console.error.

And React's hack is precisely going out of its way to cause browsers to think it's an uncaught error, even though it is actually "caugh". See the big comment at https://github.com/facebook/react/blob/3e94bce765d355d74f6a60feb4addb6d196e3482/packages/shared/invokeGuardedCallbackImpl.js#L32

So that explains why a "jsdomError" is reported. It's a combination of the intentional behavior of both jsdom and React's hack.

sjrd commented 4 years ago

@japgolly I managed to minimize the lifecycle1 test case to the following, which still exhibits the bug, and still passes with my counter-hack:

    "lifecycle1" - {
      class Props(val a: Int)

      val Inner = ScalaComponent.builder[Props]("")
        .stateless
        .render_P(p => raw.React.createElement("div", null, p.a.toString()))
        .build

      val Comp = ScalaComponent.builder[Props]("")
        .initialState[Option[String]](None) // error message
        .render_PS((p, s) => s match {
          case None    => Inner(p).vdomElement
          case Some(e) => raw.React.createElement("div", null, "Error: " + e)
        })
        .componentDidCatch($ => $.setState(Some($.error.message.replaceFirst("'.+' *", ""))))
        .build

      val staleDomNodeCallback = ReactTestUtils.withNewBodyElement { mountNode =>
        val comp = Comp(null)
        println("OK HERE WE GO.........")
        val mounted = comp.renderIntoDOM(mountNode) // <---------------------------- TROUBLE
        println("WOW IT WORKED!")

        mounted.withEffectsPure.getDOMNode
      }

      staleDomNodeCallback.runNow()
    }

Would you be able to give me a JavaScript-only version of the above logic? Something that doesn't use Scala.js at all (and hence not scalajs-react); just plain React.js.

That would be a test case I would be able to include in the test suite of scalajs-env-jsdom-nodejs, for a PR that fixes this issue.

sjrd commented 4 years ago

OK, I've got a reproducing test case, and a hack that fixes it. Just leaving it here FTR; I'll send a PR tomorrow:

  @Test
  def reactUnhandledExceptionHack: Unit = {
    val code =
      """
      |var rootElement = document.createElement("div");
      |document.body.appendChild(rootElement);
      |
      |class BasicComponent extends React.Component {
      |  render() {
      |    throw new Error("boom");
      |    return React.createElement("p", null, "Content");
      |  }
      |}
      |
      |class ErrorBoundary extends React.Component {
      |  constructor(props) {
      |    super(props);
      |    this.state = { hasError: false };
      |  }
      |
      |  componentDidCatch(error, info) {
      |    // Display fallback UI
      |    this.setState({ hasError: true });
      |    // You can also log the error to an error reporting service
      |    //logErrorToMyService(error, info);
      |  }
      |
      |  render() {
      |    if (this.state.hasError) {
      |      // You can render any custom fallback UI
      |      //throw new Error("reboom");
      |      console.log("render-error");
      |      return React.createElement("h1", null, "Something went wrong");
      |    }
      |    return this.props.children;
      |  }
      |}
      |
      |class MyMainComponent extends React.Component {
      |  constructor(...args) {
      |    super(...args);
      |    console.log("constr");
      |  }
      |  render() {
      |    console.log("two");
      |    //throw new Error("main error");
      |    return React.createElement("div", null,
      |      React.createElement(ErrorBoundary, null,
      |        React.createElement(BasicComponent)
      |      )
      |    );
      |  }
      |}
      |
      |console.log("one");
      |
      |ReactDOM.render(
      |  React.createElement(MyMainComponent, null),
      |  rootElement
      |);
      |
      |setTimeout(function() { console.log("end"); }, 1000);
      """.stripMargin

    kit.withRun(ReactJSFiles :+ codeToInput(code)) {
      _.expectOut("one\nconstr\ntwo\nrender-error\nend\n")
        .closeRun()
    }
  }
japgolly commented 4 years ago

Sure:

class Inner extends React.Component {
  render() {
    let p = this.props;
    p = p.minus(p);
    return null;
  }
}

class Comp extends React.Component {
  constructor(props) {
    super(props);
    this.state = { hasError: false };
  }

  render() {
    if (this.state.hasError)
      return React.createElement("div", null, `Error: ${this.state.error}`)
    else
      return React.createElement(Inner, this.props)
  }

  componentDidCatch(e) {
    this.setState({error: e.message, hasError: true})
  }
}

const root = document.createElement('div');
const props = React.createElement(Comp, null)
const mounted = ReactDOM.render(props, root)
japgolly commented 4 years ago

Oh whoops, I should've read all replies before I replied. :)

sjrd commented 4 years ago

Oh whoops, I should've read all replies before I replied. :)

At least it confirms that I had understood the various pieces. 😅