Error boundary catches errors of child components

[bxt]

Expected behavior

An error thrown by a child of the intersection observer should end up in the user deifned error boundary that was placed outside of the intersection observer.

Current behavior

The intersection observer catches all errors, istead of only handling a missing DOM node situation.

Steps to reproduce

Render an app like this:

import React from 'react';
import IntersectionObserver from '@researchgate/react-intersection-observer';


class MyErrorBoundary extends React.Component {
  state = {hasError: false};

  componentDidCatch(error, info) {
    console.log('I am not called!');
  }

  static getDerivedStateFromError(error) {
    return {hasError: true};
  }

  render() {
    console.log('I am called?');

    return this.state.hasError ? 'oy' : this.props.children;
  }
}

class ChildWithError extends React.Component {
  render() {
    return (
      <button
        onClick={() => {
          this.setState(() => {
            throw new Error('Some forbidden Button was clicked! ');
          });
        }}
      >
        click here to trigger error
      </button>
    );
  }
}

class App extends React.Component {
  render() {
    return (
      <MyErrorBoundary>
        <IntersectionObserver onChange={() => {}} threshold={0.5}>
          <div>
            <ChildWithError />
          </div>
        </IntersectionObserver>
      </MyErrorBoundary>
    );
  }
}

You will find the console.log('I am not called!'); is not called.

Context (environment)

• Version: 1.1.0 / 1.1.1
• Platform: Darwin host 18.7.0 Darwin Kernel Version 18.7.0: Thu Jan 23 06:52:12 PST 2020; root:xnu-4903.278.25~1/RELEASE_X86_64 x86_64

[Rendez]

I understand, would you expect the error to be bubbled up if it isn't the one about missing DOM node? That'd be easy to accomplish by rethrowing it for that case.

[bxt]

@Rendez Yeah, I think that would be a good solution, thank you for the fast response and the PR! 🎉🎉🎉

On the other hand, I'm not really sure why you brought in the error boundary, do you plan to handle more errors in the future? Otherwise maybe it could even be removed again? 🤔

[Rendez]

Good question, let me give you a little context... Our query components and async components in general render loading states, e.g:

<Observer onChange={...}>
  <Query ...>
    {null} // while loading
  </Query>
</Observer>

Often this loading state isn't a DOM node but they are null instead and during development some folks didn't see it (for example, because that component didn't render). This lead to random errors on observe() calls. The error stack traces and component stack traces of IntersectionObserver.prototype.observe were very deep, and our devs didn't see initially why these problems appeared, so we could not easily trace back to where errors happened. That's why we decided to start throwing our custom error with a nicer error message.

Up until 1.0.5 we didn't throw but instead of just called the error reporter, and we saw a new error in production (our custom reporter). However, our error was missing the info.componentStack object that one gets from componentDidCatch, so again we couldn't trace which of the many instances was causing the issue, and it wasn't that easy to reproduce locally (A/B tests etc).

And this is the story of how we ended up with error boundaries on >=1.1.0.

By the way, the reported errors will still appear in the console, whether you config the errorReporter or not. If we didn't add our own error boundary, you would need to implement reporting on your error boundaries. That would be fine, if you chose to, we had that before, but again the error message wasn't too clear, and the amount of unmounted nodes was too big.

If you feel this is too protective from the library, then I am interested to know if you'd prefer to have two exports: one guarded observer and one raw one?

[Rendez]

🎉 This issue has been resolved in version 1.1.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[bxt]

I experimented with the new version for a bit, but I could not manage to have the error propagated to our error boundary. I'm not sure what the reason is. Previously (1.1.1) nothing happened, now with 1.1.2, the component just re-mounts but does not trigger our error behavior.

We display a crash message to the user and hide the faulty part of the page, since it is likely that the error will just happen again on re-mount. That's why we can not just call it a day and have IntersectionObserver handle the errors.

What is confusing to me is the React docs on error boundaries do not mention re-throwing errors, or how a library component would use an error boundary without affecting unrelated children. I also did a bit of research and could not find anyone else doing something like this. So I think it is a bit unexpected that a library component introduces an error boundary.

I decided to work around the error boundary of the intersection observer by doing:

import {IntersectionObserver} from '@researchgate/react-intersection-observer/lib/es/IntersectionObserver';

I will discuss my workaround with the team and see how it works in production.

If you would find a way to add the IntersectionObserver without error boundary to your public API, I think that would be great, because this kind of "deeplinking" into packages is usually unexpected. If you tell me how you would call the exported component I could even provide a PR.

[Rendez]

I experimented with the new version for a bit, but I could not manage to have the error propagated to our error boundary. I'm not sure what the reason is. Previously (1.1.1) nothing happened, now with 1.1.2, the component just re-mounts but does not trigger our error behavior.

That is very strange, I added a test case specifically to prove that the parent error boundary would receive the propagated error.

Can you confirm that at least it tries to re-render the component? I didn't try this into production though, but my guess (if the re-render happens for you too) is that the production build doesn't re-throw then, which would be a bit discouraging as that's the point of "re-throwing".

After you give me some more feedback, I think I will cut a release where the default export does no include an error boundary, and perhaps even remove it

[bxt]

Okay, super strange: after comparing your test case and then our code and debugging a bit more, I found the reason is apparently that we throw the error in a state update. To test our error boundaries, Sentry integration etc. we have a button in a debugging component. And like in the code I posted above a this.setState throws an error. When I moved the error throwing into the render() directly for testing, somehow your error boundary started to re-throw the error.

In both cases it re-renders the component (re-mounts it even I think, because some internal state is reset) so I guess this is really an edge-case and can be ignored? On the other hand, since we did not see any missing node errors before, I think we could still got with the version without the error boundary 🤔

[Rendez]

That makes sense, error boundaries only catch errors during render. That is within render, cDM, cDU and constructor (I think). The re-mount happens simply because the error boundary we create simply re-renders the passed this.props.children, your children. These contain now a catchable error.

That said, you're right: I tried your example above (throwing in setState) and what's happening is, the error doesn't get bubbled up because on a re-render there is no following onClick trigger. It has to be a render error for it to be re-thrown automatically. That tells me this is a brittle mechanism and we shouldn't be catching anything in the library. It's unfortunate, but all we can do is to throw our custom error with a better message (back to the original idea).

I'll cut a release without error boundaries as soon as I can. Thanks a lot for your collaboration on this!

[Rendez]

https://github.com/researchgate/react-intersection-observer/releases/tag/v1.1.3

[bxt]

@Rendez Hey, thanks again for fixing this! Since we have this running in production for some time, I think it works perfectly. We actually have a "enterprise wrapper component" around the library which always inserts a <div>, so that's why we never run into these exceptions in the first place. Anyways, this library saves us a lot of time so thank you for making it publicly available and also responding to feeback so quickly! ❤️

[Rendez]

That is a great way to use this. Thank you!