Skip to content

Commit

Permalink
fix(boundary): DOM missing throws; no error boundary
Browse files Browse the repository at this point in the history
  • Loading branch information
@Rendez
Rendez committed Apr 9, 2020
1 parent 3bc825b commit a96c319
Show file tree
Hide file tree
Showing 6 changed files with 14 additions and 269 deletions.
52 changes: 8 additions & 44 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ all the imperative parts for you.
<!-- START doctoc generated TOC please keep comment here to allow auto update -->
<!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE -->


- [Getting started](#getting-started)
- [What does IntersectionObserver do?](#what-does-intersectionobserver-do)
- [Why use this component?](#why-use-this-component)
Expand All @@ -41,7 +42,7 @@ all the imperative parts for you.
- [Documentation](#documentation)
- [Demos](#demos)
- [Recipes](#recipes)
- [Handling a missing DOM node situation](#handling-a-missing-dom-node-situation)
- [Missing DOM nodes when observing](#missing-dom-nodes-when-observing)
- [Options](#options)
- [Notes](#notes)
- [Polyfill](#polyfill)
Expand Down Expand Up @@ -215,7 +216,7 @@ export default () => (

Discover more recipes in our [examples section](docs/README.md).

### Handling a missing DOM node situation
### Missing DOM nodes when observing

In cases where there isn't a DOM node available to observe when rendering,
you'll be seeing an error logged in the console:
Expand All @@ -227,49 +228,12 @@ ReactIntersectionObserver: Can't find DOM node in the provided children. Make su
This somewhat helpful and descriptive message is supposed to help you identify
potential problems implementing `observers` early on. If you miss the exception
for some reason and ends up in production (prone to happen with dynamic
children), this component will NOT unmount. Instead, it will gracefully catch
the error and re-render the children so that you can do custom logging and
report it. For example:
```js
import { Config } from '@researchgate/react-intersection-observer';
if (process.env.NODE_ENV === 'production') {
Config.errorReporter(function(error) {
sendReport(error);
});
}
```
Maybe you want to deal with the error on your own, for example, by rendering a
fallback. In that case, you can re-throw the error so that it bubbles up to the
next boundary:
```js
import { Config } from '@researchgate/react-intersection-observer';
Config.errorReporter(function(error) {
throw error;
});
```
children), the entire tree will unmount so be sensible about placing your error
boundaries.
While sometimes this error happens during mount, and it's easy to spot, often
types of errors happen during tree updates, because some child component that
was previously observed suddently ceaces to exist in the UI. This usually means
that either you shouldn't have rendered an `<Observer>` around it anymore or,
you should have used the `disabled` property. That's why we capture errors and
do re-rendering of the children as a fallback.

If another kind of error happens, the `errorReporter` won't be invoked, and by
rendering the children the error will bubble up to the nearest error boundary
you defined.
At [ResearchGate](www.researchgate.net), we have found that not unmounting the
tree just because we failed to `observe()` a DOM node suits our use cases
better. It's fairly common having a lack of error boundaries around your
components, and that leads to entire UIs parts being unmounted, which is not
ideal to end users. By capturing errors, we are able to keep the UI unbroken
while we fix them.
Ultimately the way to avoid this is to either make sure you are rendering a DOM
node inside your `<Observer>`, or to disable the observer until there's one
`<Observer disabled>`.

### Options

Expand Down
56 changes: 5 additions & 51 deletions src/IntersectionObserver.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,12 @@ import { findDOMNode } from 'react-dom';
import PropTypes from 'prop-types';
import { createObserver, observeElement, unobserveElement } from './observer';
import { shallowCompare } from './utils';
import Config from './config';

const observerOptions = ['root', 'rootMargin', 'threshold'];
const observableProps = ['root', 'rootMargin', 'threshold', 'disabled'];
const { hasOwnProperty, toString } = Object.prototype;
const missingNodeError = new Error(
"ReactIntersectionObserver: Can't find DOM node in the provided children. Make sure to render at least one DOM node in the tree."
);

const getOptions = (props) => {
export const getOptions = (props) => {
return observerOptions.reduce((options, key) => {
if (hasOwnProperty.call(props, key)) {
const rootIsString =
Expand All @@ -26,7 +22,7 @@ const getOptions = (props) => {
}, {});
};

class IntersectionObserver extends React.Component {
export default class IntersectionObserver extends React.Component {
static displayName = 'IntersectionObserver';

static propTypes = {
Expand Down Expand Up @@ -113,7 +109,9 @@ class IntersectionObserver extends React.Component {
return false;
}
if (!this.targetNode) {
throw missingNodeError;
throw new Error(
"ReactIntersectionObserver: Can't find DOM node in the provided children. Make sure to render at least one DOM node in the tree."
);
}
this.observer = createObserver(getOptions(this.props));
this.target = this.targetNode;
Expand Down Expand Up @@ -183,47 +181,3 @@ class IntersectionObserver extends React.Component {
: null;
}
}

class ErrorBoundary extends React.Component {
static displayName = 'ErrorBoundary(IntersectionObserver)';

static propTypes = {
forwardedRef: PropTypes.oneOfType([PropTypes.func, PropTypes.object]),
};

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

state = {
hasError: false,
};

componentDidCatch(error, info) {
if (error === missingNodeError) {
Config.errorReporter && Config.errorReporter(error, info);
}
}

render() {
const { forwardedRef, ...props } = this.props;

if (this.state.hasError) {
return props.children;
}

return <IntersectionObserver ref={forwardedRef} {...props} />;
}
}

const GuardedIntersectionObserver = React.forwardRef((props, ref) => (
<ErrorBoundary forwardedRef={ref} {...props} />
));

GuardedIntersectionObserver.displayName = 'IntersectionObserver';

export {
GuardedIntersectionObserver as default,
IntersectionObserver,
getOptions,
};
129 changes: 1 addition & 128 deletions src/__tests__/IntersectionObserver.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,8 @@
import 'intersection-observer';
import React, { Component } from 'react';
import renderer from 'react-test-renderer';
import GuardedIntersectionObserver, {
IntersectionObserver,
getOptions,
} from '../IntersectionObserver';
import IntersectionObserver, { getOptions } from '../IntersectionObserver';
import { callback, observerElementsMap } from '../observer';
import Config from '../config';

jest.mock('react-dom', () => {
const { findDOMNode } = jest.requireActual('react-dom');
Expand All @@ -22,8 +18,6 @@ jest.mock('react-dom', () => {
},
};
});
// the default "undefined" can't be re-assigned, so we preemptively set it as an empty function
Config.errorReporter = function() {};

const target = { nodeType: 1, type: 'span' };
const targets = { div: { nodeType: 1, type: 'div' }, span: target };
Expand Down Expand Up @@ -76,127 +70,6 @@ test('throws trying to observe children without a DOM node', () => {
expect(observerElementsMap.size).toBe(sizeBeforeObserving);
});

test('reports error trying to observe children without a DOM node', () => {
global.spyOn(console, 'error'); // suppress error boundary warning
const sizeBeforeObserving = observerElementsMap.size;
const originalErrorReporter = Config.errorReporter;
const spy = jest.fn();
Config.errorReporter = spy;

const tree = renderer
.create(
<GuardedIntersectionObserver onChange={noop}>
<ProxyComponent>{null}</ProxyComponent>
</GuardedIntersectionObserver>
)
.toTree();

expect(observerElementsMap.size).toBe(sizeBeforeObserving);
expect(spy).toBeCalledTimes(1);
expect(spy).toBeCalledWith(
expect.any(Error),
expect.objectContaining({ componentStack: expect.any(String) })
);
// Tree stayed mounted because of the error boundary
expect(tree.props.children.type).toEqual(ProxyComponent);

Config.errorReporter = originalErrorReporter;
});

test('reports errors by re-throwing trying observer children without a DOM node', () => {
global.spyOn(console, 'error'); // suppress error boundary warning
const originalErrorReporter = Config.errorReporter;
let called = false;
Config.errorReporter = (err) => {
called = true;
throw err;
};
class TestErrorBoundary extends React.Component {
state = { hasError: false };

componentDidCatch() {
this.setState({ hasError: true });
}

render() {
// eslint-disable-next-line react/prop-types
return this.state.hasError ? 'has-error' : this.props.children;
}
}

const children = renderer
.create(
<TestErrorBoundary>
<GuardedIntersectionObserver onChange={noop}>
<ProxyComponent>{null}</ProxyComponent>
</GuardedIntersectionObserver>
</TestErrorBoundary>
)
.toJSON();

// Tree changed because of the custom error boundary
expect(children).toBe('has-error');
expect(called).toBe(true);

Config.errorReporter = originalErrorReporter;
});

test('render a fallback when some unexpected error happens', () => {
global.spyOn(console, 'error'); // suppress error boundary warning
const originalErrorReporter = Config.errorReporter;
const spy = jest.fn();
Config.errorReporter = spy;
class TestErrorBoundary extends React.Component {
state = { hasError: false };

componentDidCatch() {
this.setState({ hasError: true });
}

render() {
// eslint-disable-next-line react/prop-types
return this.state.hasError ? 'has-error' : this.props.children;
}
}

const Boom = () => {
throw new Error('unexpected rendering error');
};

const children = renderer
.create(
<TestErrorBoundary>
<GuardedIntersectionObserver onChange={noop}>
<Boom />
</GuardedIntersectionObserver>
</TestErrorBoundary>
)
.toJSON();

// Tree changed because of the custom error boundary
expect(children).toBe('has-error');
expect(spy).not.toBeCalled();

Config.errorReporter = originalErrorReporter;
});

test('error boundary forwards ref', () => {
let observer;
renderer.create(
<GuardedIntersectionObserver
onChange={noop}
ref={(instance) => {
observer = instance;
}}
>
<div />
</GuardedIntersectionObserver>,
{ createNodeMock }
);

expect(observer instanceof IntersectionObserver).toBe(true);
});

test('should not observe children that equal null or undefined', () => {
const sizeBeforeObserving = observerElementsMap.size;
renderer.create(
Expand Down
27 changes: 0 additions & 27 deletions src/__tests__/config.spec.js
View file

This file was deleted.

18 changes: 0 additions & 18 deletions src/config.js
View file

This file was deleted.

1 change: 0 additions & 1 deletion src/index.js
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
export { default } from './IntersectionObserver';
export { parseRootMargin } from './utils';
export { default as Config } from './config';

0 comments on commit a96c319

Please sign in to comment.