Skip to content

Commit

Permalink
[Fiber] Don't Rethrow Errors at the Root (#28627)
Browse files Browse the repository at this point in the history
Stacked on top of #28498 for test fixes.

### Don't Rethrow

When we started React it was 1:1 setState calls a series of renders and
if they error, it errors where the setState was called. Simple. However,
then batching came and the error actually got thrown somewhere else.
With concurrent mode, it's not even possible to get setState itself to
throw anymore.

In fact, all APIs that can rethrow out of React are executed either at
the root of the scheduler or inside a DOM event handler.
If you throw inside a React.startTransition callback that's sync, then
that will bubble out of the startTransition but if you throw inside an
async callback or a useTransition we now need to handle it at the hook
site. So in 19 we need to make all React.startTransition swallow the
error (and report them to reportError).

The only one remaining that can throw is flushSync but it doesn't really
make sense for it to throw at the callsite neither because batching.
Just because something rendered in this flush doesn't mean it was
rendered due to what was just scheduled and doesn't mean that it should
abort any of the remaining code afterwards. setState is fire and forget.
It's send an instruction elsewhere, it's not part of the current
imperative code.

Error boundaries never rethrow. Since you should really always have
error boundaries, most of the time, it wouldn't rethrow anyway.

Rethrowing also actually currently drops errors on the floor since we
can only rethrow the first error, so to avoid that we'd need to call
reportError anyway. This happens in RN events.

The other issue with rethrowing is that it logs an extra console.error.
Since we're not sure that user code will actually log it anywhere we
still log it too just like we do with errors inside error boundaries
which leads all of these to log twice.
The goal of this PR is to never rethrow out of React instead, errors
outside of error boundaries get logged to reportError. Event system
errors too.

### Breaking Changes

The main thing this affects is testing where you want to inspect the
errors thrown. To make it easier to port, if you're inside `act` we
track the error into act in an aggregate error and then rethrow it at
the root of `act`. Unlike before though, if you flush synchronously
inside of act it'll still continue until the end of act before
rethrowing.

I expect most user code breakages would be to migrate from `flushSync`
to `act` if you assert on throwing.

However, in the React repo we also have `internalAct` and the
`waitForThrow` helpers. Since these have to use public production
implementations we track these using the global onerror or process
uncaughtException. Unlike regular act, includes both event handler
errors and onRecoverableError by default too. Not just render/commit
errors. So I had to account for that in our tests.

We restore logging an extra log for uncaught errors after the main log
with the component stack in it. We use `console.warn`. This is not yet
ignorable if you preventDefault to the main error event. To avoid
confusion if you don't end up logging the error to console I just added
`An error occurred`.

### Polyfill

All browsers we support really supports `reportError` but not all test
and server environments do, so I implemented a polyfill for browser and
node in `shared/reportGlobalError`. I don't love that this is included
in all builds and gets duplicated into isomorphic even though it's not
actually needed in production. Maybe in the future we can require a
polyfill for this.

### Follow Ups

In a follow up, I'll make caught vs uncaught error handling be
configurable too.

---------

Co-authored-by: Ricky Hanlon <rickhanlonii@gmail.com>

DiffTrain build for commit 6786563.
  • Loading branch information
sebmarkbage committed Mar 27, 2024
1 parent f3180a2 commit 4ec0582
Show file tree
Hide file tree
Showing 13 changed files with 1,903 additions and 1,740 deletions.

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @noflow
* @nolint
* @preventMunge
* @generated SignedSource<<389738660a4f2ab62588a2951e5a2988>>
* @generated SignedSource<<ba8e03c28352119146c3eb29b5683ded>>
*/

"use strict";
Expand All @@ -26,7 +26,7 @@ if (__DEV__) {
}
var dynamicFlagsUntyped = require("ReactNativeInternalFeatureFlags");

var ReactVersion = "19.0.0-canary-4322682e";
var ReactVersion = "19.0.0-canary-66632b76";

// ATTENTION
// When adding new symbols to this file,
Expand Down Expand Up @@ -100,7 +100,9 @@ if (__DEV__) {
// Tracks whether something called `use` during the current batch of work.
// Determines whether we should yield to microtasks to unwrap already resolved
// promises without suspending.
didUsePromise: false
didUsePromise: false,
// Track first uncaught error within this act
thrownErrors: []
};

/**
Expand Down Expand Up @@ -3133,6 +3135,45 @@ if (__DEV__) {
}
}

var reportGlobalError =
typeof reportError === "function" // In modern browsers, reportError will dispatch an error event,
? // emulating an uncaught JavaScript error.
reportError
: function (error) {
if (
typeof window === "object" &&
typeof window.ErrorEvent === "function"
) {
// Browser Polyfill
var message =
typeof error === "object" &&
error !== null &&
typeof error.message === "string" // eslint-disable-next-line react-internal/safe-string-coercion
? String(error.message) // eslint-disable-next-line react-internal/safe-string-coercion
: String(error);
var event = new window.ErrorEvent("error", {
bubbles: true,
cancelable: true,
message: message,
error: error
});
var shouldLog = window.dispatchEvent(event);

if (!shouldLog) {
return;
}
} else if (
typeof process === "object" && // $FlowFixMe[method-unbinding]
typeof process.emit === "function"
) {
// Node Polyfill
process.emit("uncaughtException", error);
return;
} // eslint-disable-next-line react-internal/no-production-logging

console["error"](error);
};

function startTransition(scope, options) {
var prevTransition = ReactCurrentBatchConfig.transition; // Each renderer registers a callback to receive the return value of
// the scope function. This is used to implement async actions.
Expand Down Expand Up @@ -3160,10 +3201,10 @@ if (__DEV__) {
callbacks.forEach(function (callback) {
return callback(currentTransition, returnValue);
});
returnValue.then(noop, onError);
returnValue.then(noop, reportGlobalError);
}
} catch (error) {
onError(error);
reportGlobalError(error);
} finally {
warnAboutTransitionSubscriptions(prevTransition, currentTransition);
ReactCurrentBatchConfig.transition = prevTransition;
Expand Down Expand Up @@ -3201,18 +3242,7 @@ if (__DEV__) {
}
}

function noop() {} // Use reportError, if it exists. Otherwise console.error. This is the same as
// the default for onRecoverableError.

var onError =
typeof reportError === "function" // In modern browsers, reportError will dispatch an error event,
? // emulating an uncaught JavaScript error.
reportError
: function (error) {
// In older browsers and test environments, fallback to console.error.
// eslint-disable-next-line react-internal/no-production-logging
console["error"](error);
};
function noop() {}

var didWarnAboutMessageChannel = false;
var enqueueTaskImpl = null;
Expand Down Expand Up @@ -3261,6 +3291,16 @@ if (__DEV__) {
var actScopeDepth = 0; // We only warn the first time you neglect to await an async `act` scope.

var didWarnNoAwaitAct = false;

function aggregateErrors(errors) {
if (errors.length > 1 && typeof AggregateError === "function") {
// eslint-disable-next-line no-undef
return new AggregateError(errors);
}

return errors[0];
}

function act(callback) {
{
// When ReactCurrentActQueue.current is not null, it signals to React that
Expand Down Expand Up @@ -3312,9 +3352,15 @@ if (__DEV__) {
// one used to track `act` scopes. Why, you may be wondering? Because
// that's how it worked before version 18. Yes, it's confusing! We should
// delete legacy mode!!
ReactCurrentActQueue.thrownErrors.push(error);
}

if (ReactCurrentActQueue.thrownErrors.length > 0) {
ReactCurrentActQueue.isBatchingLegacy = prevIsBatchingLegacy;
popActScope(prevActQueue, prevActScopeDepth);
throw error;
var thrownError = aggregateErrors(ReactCurrentActQueue.thrownErrors);
ReactCurrentActQueue.thrownErrors.length = 0;
throw thrownError;
}

if (
Expand Down Expand Up @@ -3369,15 +3415,34 @@ if (__DEV__) {
// `thenable` might not be a real promise, and `flushActQueue`
// might throw, so we need to wrap `flushActQueue` in a
// try/catch.
reject(error);
ReactCurrentActQueue.thrownErrors.push(error);
}

if (ReactCurrentActQueue.thrownErrors.length > 0) {
var _thrownError = aggregateErrors(
ReactCurrentActQueue.thrownErrors
);

ReactCurrentActQueue.thrownErrors.length = 0;
reject(_thrownError);
}
} else {
resolve(returnValue);
}
},
function (error) {
popActScope(prevActQueue, prevActScopeDepth);
reject(error);

if (ReactCurrentActQueue.thrownErrors.length > 0) {
var _thrownError2 = aggregateErrors(
ReactCurrentActQueue.thrownErrors
);

ReactCurrentActQueue.thrownErrors.length = 0;
reject(_thrownError2);
} else {
reject(error);
}
}
);
}
Expand Down Expand Up @@ -3430,6 +3495,15 @@ if (__DEV__) {
ReactCurrentActQueue.current = null;
}

if (ReactCurrentActQueue.thrownErrors.length > 0) {
var _thrownError3 = aggregateErrors(
ReactCurrentActQueue.thrownErrors
);

ReactCurrentActQueue.thrownErrors.length = 0;
throw _thrownError3;
}

return {
then: function (resolve, reject) {
didAwaitActCall = true;
Expand Down Expand Up @@ -3486,15 +3560,21 @@ if (__DEV__) {
reject
);
});
return;
} catch (error) {
// Leave remaining tasks on the queue if something throws.
reject(error);
ReactCurrentActQueue.thrownErrors.push(error);
}
} else {
// The queue is empty. We can finish.
ReactCurrentActQueue.current = null;
resolve(returnValue);
}
}

if (ReactCurrentActQueue.thrownErrors.length > 0) {
var thrownError = aggregateErrors(ReactCurrentActQueue.thrownErrors);
ReactCurrentActQueue.thrownErrors.length = 0;
reject(thrownError);
} else {
resolve(returnValue);
}
Expand Down Expand Up @@ -3539,7 +3619,7 @@ if (__DEV__) {
} catch (error) {
// If something throws, leave the remaining callbacks on the queue.
queue.splice(0, i + 1);
throw error;
ReactCurrentActQueue.thrownErrors.push(error);
} finally {
isFlushing = false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @noflow
* @nolint
* @preventMunge
* @generated SignedSource<<ccb8a31f6eb2aabad35c7b0717209352>>
* @generated SignedSource<<ac6e1c721cd9e734454c18870d36b509>>
*/

"use strict";
Expand Down Expand Up @@ -372,13 +372,36 @@ function lazyInitializer(payload) {
if (1 === payload._status) return payload._result.default;
throw payload._result;
}
function noop() {}
var onError =
var reportGlobalError =
"function" === typeof reportError
? reportError
: function (error) {
if (
"object" === typeof window &&
"function" === typeof window.ErrorEvent
) {
var event = new window.ErrorEvent("error", {
bubbles: !0,
cancelable: !0,
message:
"object" === typeof error &&
null !== error &&
"string" === typeof error.message
? String(error.message)
: String(error),
error: error
});
if (!window.dispatchEvent(event)) return;
} else if (
"object" === typeof process &&
"function" === typeof process.emit
) {
process.emit("uncaughtException", error);
return;
}
console.error(error);
};
function noop() {}
exports.Children = {
map: mapChildren,
forEach: function (children, forEachFunc, forEachContext) {
Expand Down Expand Up @@ -532,9 +555,9 @@ exports.startTransition = function (scope) {
(callbacks.forEach(function (callback) {
return callback(currentTransition, returnValue);
}),
returnValue.then(noop, onError));
returnValue.then(noop, reportGlobalError));
} catch (error) {
onError(error);
reportGlobalError(error);
} finally {
ReactCurrentBatchConfig.transition = prevTransition;
}
Expand Down Expand Up @@ -640,4 +663,4 @@ exports.useSyncExternalStore = function (
exports.useTransition = function () {
return ReactCurrentDispatcher.current.useTransition();
};
exports.version = "19.0.0-canary-b3b89a7e";
exports.version = "19.0.0-canary-bd294150";
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @noflow
* @nolint
* @preventMunge
* @generated SignedSource<<111d124bf374c5bbae728fb7f4d17857>>
* @generated SignedSource<<50a5378f60ee29fee7964f02f5243965>>
*/

"use strict";
Expand Down Expand Up @@ -339,13 +339,36 @@ function lazyInitializer(payload) {
if (1 === payload._status) return payload._result.default;
throw payload._result;
}
function noop() {}
var onError =
var reportGlobalError =
"function" === typeof reportError
? reportError
: function (error) {
if (
"object" === typeof window &&
"function" === typeof window.ErrorEvent
) {
var event = new window.ErrorEvent("error", {
bubbles: !0,
cancelable: !0,
message:
"object" === typeof error &&
null !== error &&
"string" === typeof error.message
? String(error.message)
: String(error),
error: error
});
if (!window.dispatchEvent(event)) return;
} else if (
"object" === typeof process &&
"function" === typeof process.emit
) {
process.emit("uncaughtException", error);
return;
}
console.error(error);
};
function noop() {}
exports.Children = {
map: mapChildren,
forEach: function (children, forEachFunc, forEachContext) {
Expand Down Expand Up @@ -529,9 +552,9 @@ exports.startTransition = function (scope) {
(callbacks.forEach(function (callback) {
return callback(currentTransition, returnValue);
}),
returnValue.then(noop, onError));
returnValue.then(noop, reportGlobalError));
} catch (error) {
onError(error);
reportGlobalError(error);
} finally {
ReactCurrentBatchConfig.transition = prevTransition;
}
Expand Down Expand Up @@ -636,7 +659,7 @@ exports.useSyncExternalStore = function (
exports.useTransition = function () {
return ReactCurrentDispatcher.current.useTransition();
};
exports.version = "19.0.0-canary-4b9ba4bc";
exports.version = "19.0.0-canary-5f0b765f";
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
"function" ===
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
2ec2aaea98588178525f83495669e11e96815a00
6786563f3cbbc9b16d5a8187207b5bd904386e53
Loading

0 comments on commit 4ec0582

Please sign in to comment.