Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions scripts/fiber/tests-passing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2334,15 +2334,17 @@ src/renderers/shared/utils/__tests__/ReactErrorUtils-test.js
* should catch errors (development)
* should return false from clearCaughtError if no error was thrown (development)
* can nest with same debug name (development)
* does not return nested errors (development)
* handles nested errors (development)
* handles nested errors in separate renderers
* can be shimmed (development)
* it should rethrow caught errors (production)
* should call the callback the passed arguments (production)
* should call the callback with the provided context (production)
* should catch errors (production)
* should return false from clearCaughtError if no error was thrown (production)
* can nest with same debug name (production)
* does not return nested errors (production)
* handles nested errors (production)
* handles nested errors in separate renderers
* catches null values
* can be shimmed (production)

Expand Down
2 changes: 1 addition & 1 deletion src/renderers/shared/fiber/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ if (__DEV__) {

module.exports = function<T, P, I, TI, PI, C, CX, PL>(
config: HostConfig<T, P, I, TI, PI, C, CX, PL>,
captureError: (failedFiber: Fiber, error: Error) => Fiber | null,
captureError: (failedFiber: Fiber, error: mixed) => Fiber | null,
) {
const {
commitMount,
Expand Down
33 changes: 20 additions & 13 deletions src/renderers/shared/utils/ReactErrorUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@ const invariant = require('fbjs/lib/invariant');

const ReactErrorUtils = {
// Used by Fiber to simulate a try-catch.
_caughtError: null,
_hasCaughtError: false,
_caughtError: (null: mixed),
_hasCaughtError: (false: boolean),

// Used by event system to capture/rethrow the first error.
_rethrowError: null,
_hasRethrowError: false,
_rethrowError: (null: mixed),
_hasRethrowError: (false: boolean),

injection: {
injectErrorUtils(injectedErrorUtils: Object) {
Expand Down Expand Up @@ -155,33 +155,40 @@ if (__DEV__) {
e,
f,
) {
ReactErrorUtils._hasCaughtError = false;
ReactErrorUtils._caughtError = null;

depth++;
const thisDepth = depth;

let error;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the onerror event doesn't happen (e.g. because browser flakiness, or monkey patched DOM) then error will be undefined and that will be the error that we catch.

I think that's a fine value for that case but maybe we should explicitly set that here so that we know that it's intentional and that it can have that value.

E.g.

let error = undefined;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok yeah, I'll do that and also add some comments explaining how this all works, because it's not so straightforward if you don't know what this is all for.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, please do! I remember this really tripped me first time I looked at this.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also create our own error object with a helpful message explaining what happened. If there is browser flakiness, this would be more helpful to the developer than throwing undefined.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's clever! Let's do that.

let didError = true;
const funcArgs = Array.prototype.slice.call(arguments, 3);
const boundFunc = function() {
func.apply(context, funcArgs);
didError = false;
};
const onFakeEventError = function(event) {
// Don't capture nested errors
if (depth === thisDepth) {
ReactErrorUtils._caughtError = event.error;
ReactErrorUtils._hasCaughtError = true;
}
error = event.error;
if (preventDefault) {
event.preventDefault();
}
};

const evtType = `react-${name ? name : 'invokeguardedcallback'}-${depth}`;
window.addEventListener('error', onFakeEventError);
fakeNode.addEventListener(evtType, boundFunc, false);
const evt = document.createEvent('Event');

evt.initEvent(evtType, false, false);
fakeNode.dispatchEvent(evt);
if (didError) {
ReactErrorUtils._hasCaughtError = true;
ReactErrorUtils._caughtError = error;
} else {
ReactErrorUtils._hasCaughtError = false;
ReactErrorUtils._caughtError = null;
}

fakeNode.removeEventListener(evtType, boundFunc, false);
window.removeEventListener('error', onFakeEventError);

depth--;
};

Expand Down
33 changes: 32 additions & 1 deletion src/renderers/shared/utils/__tests__/ReactErrorUtils-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ describe('ReactErrorUtils', () => {
expect(err4).toBe(err3);
});

it(`does not return nested errors (${environment})`, () => {
it(`handles nested errors (${environment})`, () => {
const err1 = new Error();
let err2;
ReactErrorUtils.invokeGuardedCallback(
Expand All @@ -155,6 +155,37 @@ describe('ReactErrorUtils', () => {
expect(err2).toBe(err1);
});

it('handles nested errors in separate renderers', () => {
const ReactErrorUtils1 = require('ReactErrorUtils');
jest.resetModules();
const ReactErrorUtils2 = require('ReactErrorUtils');
expect(ReactErrorUtils1).not.toEqual(ReactErrorUtils2);

let ops = [];

ReactErrorUtils1.invokeGuardedCallback(
null,
() => {
ReactErrorUtils2.invokeGuardedCallback(
null,
() => {
throw new Error('nested error');
},
null,
);
// ReactErrorUtils2 should catch the error
ops.push(ReactErrorUtils2.hasCaughtError());
ops.push(ReactErrorUtils2.clearCaughtError().message);
},
null,
);

// ReactErrorUtils1 should not catch the error
ops.push(ReactErrorUtils1.hasCaughtError());

expect(ops).toEqual([true, 'nested error', false]);
});

if (environment === 'production') {
// jsdom doesn't handle this properly, but Chrome and Firefox should. Test
// this with a fixture.
Expand Down