feat(errorReporting): error reporting happens now in error boundary

Rendez · Rendez · commit 11f5e9f10b30 · 2020-03-24T16:56:26.000+01:00
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -0,0 +1,2 @@
+// Keep this file for editor support
+module.exports = require('@researchgate/spire-config/eslint/react');
diff --git a/.gitignore b/.gitignore
@@ -3,4 +3,5 @@ yarn-error.log
 /node_modules
 /lib
 /src/coverage
-/.docs
+/.docs
+.DS_Store
diff --git a/README.md b/README.md
@@ -41,7 +41,7 @@ all the imperative parts for you.
 - [Documentation](#documentation)
   - [Demos](#demos)
   - [Recipes](#recipes)
-  - [Exception handling](#exception-handling)
+  - [Handling a missing DOM node situation](#handling-a-missing-dom-node-situation)
   - [Options](#options)
   - [Notes](#notes)
 - [Polyfill](#polyfill)
@@ -208,35 +208,63 @@ import ViewableMonitor from './ViewableMonitor';
 
 export default () => (
   <ViewableMonitor>
-    {isViewable => (isViewable ? 'I am viewable' : 'I am still hiding')}
+    {(isViewable) => (isViewable ? 'I am viewable' : 'I am still hiding')}
   </ViewableMonitor>
 );
 ```
 
 Discover more recipes in our [examples section](docs/README.md).
 
-### Exception handling
+### Handling a missing DOM node situation
 
-By default, an `invariant` error is thrown if there isn't a DOM node to observe
-when mounted. We've added a helpful and descriptive message that's supposed to
-help you identify potential problems early on. However, this could also
-unexpectedtly trigger in production, specially with dynamic children, causing a
-bigger problem (bubbling up) if not handled correctly (e.g.: error boundary). In
-such cases, you can set the config for the error reporter to adjust it to your
-needs:
+In cases where there isn't a DOM node available to observe when rendering,
+you'll be seeing an error logged in the console:
+
+```js
+ReactIntersectionObserver: Can't find DOM node in the provided children. Make sure to render at least one DOM node in the tree.
+```
+
+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 so that you can do custom logging and report it. For example:
 
 ```js
 import { Config } from '@researchgate/react-intersection-observer';
 
-Config.errorReporter(function(errorMsg) {
-    if (process.env.NODE_ENV === 'production') {
-        sendReport((errorMsg);
-    } else {
-        console.error(errorMsg);
-    }
+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;
 });
 ```
 
+If this error happens during mount, it's easy to spot. However, a lot of these
+errors usually 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.
+
+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.
+
 ### Options
 
 **root**: `HTMLElement|string` | default `window object`
@@ -313,7 +341,7 @@ might want to decorate your `onChange` callback with a `requestIdleCallback` or
 `setTimeout` call to avoid a potential performance degradation:
 
 ```js
-onChange = entry => requestIdleCallback(() => this.handleChange(entry));
+onChange = (entry) => requestIdleCallback(() => this.handleChange(entry));
 ```
 
 ## [**IntersectionObserver**'s Browser Support](https://platform-status.mozilla.org/)
diff --git a/package.json b/package.json
@@ -10,7 +10,6 @@
     "url": "https://github.com/researchgate/react-intersection-observer/issues"
   },
   "dependencies": {
-    "invariant": "^2.2.4",
     "prop-types": "^15.7.2"
   },
   "devDependencies": {
@@ -67,6 +66,7 @@
       "**/__tests__/**/*.spec.js"
     ]
   },
+  "prettier": "@researchgate/prettier-config",
   "spire": {
     "extends": "@researchgate/spire-config",
     "plugins": [
@@ -85,6 +85,5 @@
     "storybook": "start-storybook -p 9001 -c .storybook",
     "test": "spire test",
     "ts:check": "tsc --project types"
-  },
-  "prettier": "@researchgate/prettier-config"
+  }
 }
diff --git a/src/IntersectionObserver.js b/src/IntersectionObserver.js
@@ -110,10 +110,9 @@ class IntersectionObserver extends React.Component {
             return false;
         }
         if (!this.targetNode) {
-            Config.errorReporter(
+            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."
             );
-            return false;
         }
         this.observer = createObserver(getOptions(this.props));
         this.target = this.targetNode;
@@ -184,4 +183,22 @@ class IntersectionObserver extends React.Component {
     }
 }
 
-export { IntersectionObserver as default, getOptions };
+class GuardedIntersectionObserver extends React.Component {
+    static displayName = 'ErrorBoundary(IntersectionObserver)';
+
+    componentDidCatch(error, info) {
+        if (Config.errorReporter) {
+            Config.errorReporter(error, info);
+        }
+    }
+
+    render() {
+        return <IntersectionObserver {...this.props} />;
+    }
+}
+
+export {
+    GuardedIntersectionObserver as default,
+    IntersectionObserver,
+    getOptions,
+};
diff --git a/src/__tests__/IntersectionObserver.spec.js b/src/__tests__/IntersectionObserver.spec.js
@@ -2,7 +2,10 @@
 import 'intersection-observer';
 import React, { Component } from 'react';
 import renderer from 'react-test-renderer';
-import IntersectionObserver, { getOptions } from '../IntersectionObserver';
+import GuardedIntersectionObserver, {
+    IntersectionObserver,
+    getOptions,
+} from '../IntersectionObserver';
 import { callback, observerElementsMap } from '../observer';
 import Config from '../config';
 
@@ -19,6 +22,8 @@ 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 };
@@ -71,6 +76,71 @@ 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('should not observe children that equal null or undefined', () => {
     const sizeBeforeObserving = observerElementsMap.size;
     renderer.create(
@@ -348,37 +418,7 @@ describe('updating', () => {
         expect(observe).toBeCalledTimes(1);
     });
 
-    test('should call a setup errorReporter without a DOM Node', () => {
-        const spy = jest.fn();
-        const origErrorReporter = Config.errorReporter;
-        Config.errorReporter = spy;
-        const tree = renderer.create(
-            <IntersectionObserver onChange={noop}>
-                <ProxyComponent>
-                    <div />
-                </ProxyComponent>
-            </IntersectionObserver>,
-            { createNodeMock }
-        );
-        const instance = tree.getInstance();
-        const observe = jest.spyOn(instance, 'observe');
-        const unobserve = jest.spyOn(instance, 'unobserve');
-
-        tree.update(
-            <IntersectionObserver onChange={noop}>
-                <ProxyComponent key="forcesRender">{null}</ProxyComponent>
-            </IntersectionObserver>
-        );
-
-        expect(spy).toBeCalled();
-        expect(unobserve).toBeCalledTimes(1);
-        expect(observe).toBeCalledTimes(1);
-        expect(observe).toReturnWith(false);
-
-        Config.errorReporter = origErrorReporter;
-    });
-
-    test('should observe when updating with a DOM Node', () => {
+    test('should observe only when updating with a DOM Node', () => {
         global.spyOn(console, 'error'); // suppress error boundary warning
 
         const sizeAfterUnobserving = observerElementsMap.size;
diff --git a/src/__tests__/config.spec.js b/src/__tests__/config.spec.js
@@ -1,26 +1,20 @@
 /* eslint-env jest */
 import Config from '../config';
-import invariant from 'invariant';
-
-jest.mock('invariant', () => jest.fn());
 
 describe('Config', () => {
-    const testErrorReporter = jest.fn();
-    const errorMsg = 'Intentionally throw exception';
-
     test('default errorReporter', () => {
-        Config.errorReporter(errorMsg);
-        expect(invariant).toBeCalledWith(false, errorMsg);
+        expect(Config.errorReporter).toBeUndefined();
     });
 
     test('custom errorReporter', () => {
-        const defaultTestErrorReporter = Config.errorReporter;
-        Config.errorReporter = testErrorReporter;
-
-        Config.errorReporter(errorMsg);
-        expect(testErrorReporter).toBeCalledWith(errorMsg);
+        const testErrorReporter = jest.fn();
+        const errorMsg = 'Intentionally throw exception';
+        const infoMsg = 'Meta information for exception';
+        const InlineConfig = require('../config');
+        InlineConfig.errorReporter = testErrorReporter;
+        InlineConfig.errorReporter(errorMsg, infoMsg);
 
-        Config.errorReporter = defaultTestErrorReporter;
+        expect(testErrorReporter).toBeCalledWith(errorMsg, infoMsg);
     });
 
     test('custom non-callable errorReporter', () => {
diff --git a/src/config.js b/src/config.js
@@ -1,17 +1,10 @@
-import invariant from 'invariant';
-
 const config = {};
 
 export default Object.create(null, {
     errorReporter: {
         configurable: false,
         get() {
-            return (
-                config.errorReporter ||
-                function(format) {
-                    return invariant(false, format);
-                }
-            );
+            return config.errorReporter;
         },
         set(value) {
             if (typeof value !== 'function') {
diff --git a/yarn.lock b/yarn.lock
@@ -7241,7 +7241,6 @@ fsevents@^1.2.7:
   dependencies:
     bindings "^1.5.0"
     nan "^2.12.1"
-    node-pre-gyp "*"
 
 fsevents@^2.1.2:
   version "2.1.2"
@@ -11289,7 +11288,6 @@ npm@^6.10.3:
     cmd-shim "^3.0.3"
     columnify "~1.5.4"
     config-chain "^1.1.12"
-    debuglog "*"
     detect-indent "~5.0.0"
     detect-newline "^2.1.0"
     dezalgo "~1.0.3"
@@ -11304,7 +11302,6 @@ npm@^6.10.3:
     has-unicode "~2.0.1"
     hosted-git-info "^2.8.7"
     iferr "^1.0.2"
-    imurmurhash "*"
     infer-owner "^1.0.4"
     inflight "~1.0.6"
     inherits "^2.0.4"
@@ -11323,14 +11320,8 @@ npm@^6.10.3:
     libnpx "^10.2.2"
     lock-verify "^2.1.0"
     lockfile "^1.0.4"
-    lodash._baseindexof "*"
     lodash._baseuniq "~4.6.0"
-    lodash._bindcallback "*"
-    lodash._cacheindexof "*"
-    lodash._createcache "*"
-    lodash._getnative "*"
     lodash.clonedeep "~4.5.0"
-    lodash.restparam "*"
     lodash.union "~4.6.0"
     lodash.uniq "~4.5.0"
     lodash.without "~4.4.0"

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+// Keep this file for editor support`
	`2`	`+module.exports = require('@researchgate/spire-config/eslint/react');`