Closed bvaughn closed 5 years ago
I took a quick pass at this this afternoon but I didn't get finished. Got a little hung up on the best way to tell if an error boundary was in an "errored" state. Seems particularly tricky for "legacy boundaries" since React itself uses a Map
to track these.
So here's a dump of the partial changes I made to react:
diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js
index 18c076bd3..bc25b680d 100644
--- a/packages/react-reconciler/src/ReactFiberBeginWork.js
+++ b/packages/react-reconciler/src/ReactFiberBeginWork.js
@@ -110,7 +110,7 @@ import {
registerSuspenseInstanceRetry,
} from './ReactFiberHostConfig';
import type {SuspenseInstance} from './ReactFiberHostConfig';
-import {shouldSuspend} from './ReactFiberReconciler';
+import {shouldError, shouldSuspend} from './ReactFiberReconciler';
import {pushHostContext, pushHostContainer} from './ReactFiberHostContext';
import {
suspenseStackCursor,
@@ -606,6 +606,13 @@ function updateFunctionComponent(
nextProps: any,
renderExpirationTime,
) {
+ // This is used by DevTools to force a boundary to suspend.
+ if (__DEV__) {
+ if (shouldError(workInProgress)) {
+ workInProgress.effectTag |= DidCapture;
+ }
+ }
+
if (__DEV__) {
if (workInProgress.type !== workInProgress.elementType) {
// Lazy component props can't be validated in createElement
@@ -701,6 +708,13 @@ function updateClassComponent(
nextProps,
renderExpirationTime: ExpirationTime,
) {
+ // This is used by DevTools to force a boundary to suspend.
+ if (__DEV__) {
+ if (shouldError(workInProgress)) {
+ workInProgress.effectTag |= DidCapture;
+ }
+ }
+
if (__DEV__) {
if (workInProgress.type !== workInProgress.elementType) {
// Lazy component props can't be validated in createElement
diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js
index aade7ccb1..1d2f6366e 100644
--- a/packages/react-reconciler/src/ReactFiberReconciler.js
+++ b/packages/react-reconciler/src/ReactFiberReconciler.js
@@ -381,8 +381,13 @@ export function findHostInstanceWithNoPortals(
return hostFiber.stateNode;
}
+let shouldErrorImpl = fiber => false;
let shouldSuspendImpl = fiber => false;
+export function shouldError(fiber: Fiber): boolean {
+ return shouldErrorImpl(fiber);
+}
+
export function shouldSuspend(fiber: Fiber): boolean {
return shouldSuspendImpl(fiber);
}
@@ -390,6 +395,7 @@ export function shouldSuspend(fiber: Fiber): boolean {
let overrideHookState = null;
let overrideProps = null;
let scheduleUpdate = null;
+let setErrorHandler = null;
let setSuspenseHandler = null;
if (__DEV__) {
@@ -470,6 +476,10 @@ if (__DEV__) {
scheduleWork(fiber, Sync);
};
+ setErrorHandler = (newShouldErrorImpl: Fiber => boolean) => {
+ shouldErrorImpl = newShouldErrorImpl;
+ };
+
setSuspenseHandler = (newShouldSuspendImpl: Fiber => boolean) => {
shouldSuspendImpl = newShouldSuspendImpl;
};
@@ -483,6 +493,7 @@ export function injectIntoDevTools(devToolsConfig: DevToolsConfig): boolean {
...devToolsConfig,
overrideHookState,
overrideProps,
+ setErrorHandler,
setSuspenseHandler,
scheduleUpdate,
currentDispatcherRef: ReactCurrentDispatcher,
And here's a dump of the partial changes I made tp DevTools:
diff --git a/shells/dev/app/index.js b/shells/dev/app/index.js
index b0609a8..f4d6d1c 100644
--- a/shells/dev/app/index.js
+++ b/shells/dev/app/index.js
@@ -11,6 +11,7 @@ import DeeplyNestedComponents from './DeeplyNestedComponents';
import Iframe from './Iframe';
import EditableProps from './EditableProps';
import ElementTypes from './ElementTypes';
+import ErrorBoundaries from './ErrorBoundaries';
import Hydration from './Hydration';
import InspectableElements from './InspectableElements';
import InteractionTracing from './InteractionTracing';
@@ -42,6 +43,7 @@ function mountTestApp() {
mountHelper(Hydration);
mountHelper(ElementTypes);
mountHelper(EditableProps);
+ mountHelper(ErrorBoundaries);
mountHelper(PriorityLevels);
mountHelper(ReactNativeWeb);
mountHelper(Toggle);
diff --git a/src/backend/agent.js b/src/backend/agent.js
index a711080..10553f8 100644
--- a/src/backend/agent.js
+++ b/src/backend/agent.js
@@ -50,6 +50,12 @@ type InspectElementParams = {|
rendererID: number,
|};
+type OverrideErrorParams = {|
+ id: number,
+ rendererID: number,
+ forceError: boolean,
+|};
+
type OverrideHookParams = {|
id: number,
hookID: number,
@@ -120,6 +126,7 @@ export default class Agent extends EventEmitter<{|
bridge.addListener('inspectElement', this.inspectElement);
bridge.addListener('logElementToConsole', this.logElementToConsole);
bridge.addListener('overrideContext', this.overrideContext);
+ bridge.addListener('overrideError', this.overrideError);
bridge.addListener('overrideHookState', this.overrideHookState);
bridge.addListener('overrideProps', this.overrideProps);
bridge.addListener('overrideState', this.overrideState);
@@ -304,6 +311,19 @@ export default class Agent extends EventEmitter<{|
}
};
+ overrideError = ({
+ id,
+ rendererID,
+ forceError,
+ }: OverrideErrorParams) => {
+ const renderer = this._rendererInterfaces[rendererID];
+ if (renderer == null) {
+ console.warn(`Invalid renderer id "${rendererID}" for element "${id}"`);
+ } else {
+ renderer.overrideError(id, forceError);
+ }
+ };
+
overrideHookState = ({
id,
hookID,
diff --git a/src/backend/renderer.js b/src/backend/renderer.js
index 979eefd..2b53dd8 100644
--- a/src/backend/renderer.js
+++ b/src/backend/renderer.js
@@ -126,6 +126,7 @@ type ReactTypeOfWorkType = {|
|};
type ReactTypeOfSideEffectType = {|
+ DidCapture: number,
NoEffect: number,
PerformedWork: number,
Placement: number,
@@ -175,6 +176,7 @@ export function getInternalReactConstants(
};
const ReactTypeOfSideEffect: ReactTypeOfSideEffectType = {
+ DidCapture: 0b1000000,
NoEffect: 0b00,
PerformedWork: 0b01,
Placement: 0b10,
@@ -431,7 +433,12 @@ export function attach(
ReactSymbols,
ReactTypeOfSideEffect,
} = getInternalReactConstants(renderer.version);
- const { NoEffect, PerformedWork, Placement } = ReactTypeOfSideEffect;
+ const {
+ DidCapture,
+ NoEffect,
+ PerformedWork,
+ Placement,
+ } = ReactTypeOfSideEffect;
const {
FunctionComponent,
ClassComponent,
@@ -477,9 +484,15 @@ export function attach(
const {
overrideHookState,
overrideProps,
+ setErrorHandler,
setSuspenseHandler,
scheduleUpdate,
} = renderer;
+
+ const supportsTogglingError =
+ typeof setErrorHandler === 'function' &&
+ typeof scheduleUpdate === 'function';
+
const supportsTogglingSuspense =
typeof setSuspenseHandler === 'function' &&
typeof scheduleUpdate === 'function';
@@ -1113,6 +1126,23 @@ export function attach(
return stringID;
}
+ function isErrorBoundary(fiber: Fiber): boolean {
+ const { tag, type } = fiber;
+
+ switch (tag) {
+ case ClassComponent:
+ case IncompleteClassComponent:
+ const instance = fiber.stateNode;
+ return (
+ typeof type.getDerivedStateFromError === 'function' ||
+ (instance !== null &&
+ typeof instance.componentDidCatch === 'function')
+ );
+ default:
+ return false;
+ }
+ }
+
function recordMount(fiber: Fiber, parentFiber: Fiber | null) {
const isRoot = fiber.tag === HostRoot;
const id = getFiberID(getPrimaryFiber(fiber));
@@ -1151,6 +1181,7 @@ export function attach(
pushOperation(elementType);
pushOperation(parentID);
pushOperation(ownerID);
+ pushOperation(isErrorBoundary(fiber) ? 1 : 0);
pushOperation(displayNameStringID);
pushOperation(keyStringID);
}
@@ -2237,6 +2268,8 @@ export function attach(
}
}
+ const isErrored = false; // TODO How do we calculate this?
+
return {
id,
@@ -2246,6 +2279,14 @@ export function attach(
// Does the current renderer support editable function props?
canEditFunctionProps: typeof overrideProps === 'function',
+ canToggleError:
+ supportsTogglingError &&
+ // If it's showing the real content, we can always flip it into an error state.
+ (!isErrored ||
+ // If it's showing an error state because we previously forced it to,
+ // allow toggling it back to remove the error boundary.
+ forceErrorForFiberIDs.has(id)),
+
canToggleSuspense:
supportsTogglingSuspense &&
// If it's showing the real content, we can always flip fallback.
@@ -2257,6 +2298,9 @@ export function attach(
// Can view component source location.
canViewSource,
+ // Is this element an error boundary currently in an errored state?
+ isErrored,
+
displayName: getDisplayNameForFiber(fiber),
type: getElementTypeForFiber(fiber),
@@ -2705,16 +2749,52 @@ export function attach(
// React will switch between these implementations depending on whether
// we have any manually suspended Fibers or not.
+ function shouldErrorFiberAlwaysFalse() {
+ return false;
+ }
+
function shouldSuspendFiberAlwaysFalse() {
return false;
}
+ let forceErrorForFiberIDs = new Set();
+ function shouldErrorFiberAccordingToSet(fiber) {
+ const id = getFiberID(getPrimaryFiber(((fiber: any): Fiber)));
+ return forceErrorForFiberIDs.has(id);
+ }
+
let forceFallbackForSuspenseIDs = new Set();
function shouldSuspendFiberAccordingToSet(fiber) {
const id = getFiberID(getPrimaryFiber(((fiber: any): Fiber)));
return forceFallbackForSuspenseIDs.has(id);
}
+ function overrideError(id, forceError) {
+ if (
+ typeof setErrorHandler !== 'function' ||
+ typeof scheduleUpdate !== 'function'
+ ) {
+ throw new Error(
+ 'Expected overrideError() to not get called for earlier React versions.'
+ );
+ }
+ if (forceError) {
+ forceErrorForFiberIDs.add(id);
+ if (forceErrorForFiberIDs.size === 1) {
+ // First override is added. Switch React to slower path.
+ setErrorHandler(shouldErrorFiberAccordingToSet);
+ }
+ } else {
+ forceErrorForFiberIDs.delete(id);
+ if (forceErrorForFiberIDs.size === 0) {
+ // Last override is gone. Switch React back to fast path.
+ setErrorHandler(shouldErrorFiberAlwaysFalse);
+ }
+ }
+ const fiber = idToFiberMap.get(id);
+ scheduleUpdate(fiber);
+ }
+
function overrideSuspense(id, forceFallback) {
if (
typeof setSuspenseHandler !== 'function' ||
@@ -2984,6 +3064,7 @@ export function attach(
inspectElement,
logElementToConsole,
prepareViewElementSource,
+ overrideError,
overrideSuspense,
renderer,
selectElement,
diff --git a/src/backend/types.js b/src/backend/types.js
index c0a2722..4719a5b 100644
--- a/src/backend/types.js
+++ b/src/backend/types.js
@@ -134,6 +134,7 @@ export type ReactRenderer = {
// 16.9+
scheduleUpdate?: ?(fiber: Object) => void,
+ setErrorHandler?: ?(shouldError: (fiber: Object) => boolean) => void,
setSuspenseHandler?: ?(shouldSuspend: (fiber: Object) => boolean) => void,
// Only injected by React v16.8+ in order to support hooks inspection.
diff --git a/src/devtools/store.js b/src/devtools/store.js
index adaccfe..f8847fb 100644
--- a/src/devtools/store.js
+++ b/src/devtools/store.js
@@ -756,6 +756,7 @@ export default class Store extends EventEmitter<{|
);
}
+ let isErrorBoundary: boolean = false;
let ownerID: number = 0;
let parentID: number = ((null: any): number);
if (type === ElementTypeRoot) {
@@ -798,6 +799,9 @@ export default class Store extends EventEmitter<{|
ownerID = ((operations[i]: any): number);
i++;
+ isErrorBoundary = ((operations[i]: any): number) > 0;
+ i++;
+
const displayNameStringID = operations[i];
const displayName = stringTable[displayNameStringID];
i++;
@@ -836,6 +840,7 @@ export default class Store extends EventEmitter<{|
hocDisplayNames,
id,
isCollapsed: this._collapseNodesByDefault,
+ isErrorBoundary,
key,
ownerID,
parentID: parentElement.id,
diff --git a/src/devtools/views/ButtonIcon.js b/src/devtools/views/ButtonIcon.js
index 6c3887f..0bf28c4 100644
--- a/src/devtools/views/ButtonIcon.js
+++ b/src/devtools/views/ButtonIcon.js
@@ -12,6 +12,7 @@ export type IconType =
| 'copy'
| 'delete'
| 'down'
+ | 'error'
| 'expanded'
| 'export'
| 'filter'
@@ -63,6 +64,9 @@ export default function ButtonIcon({ className = '', type }: Props) {
case 'down':
pathData = PATH_DOWN;
break;
+ case 'error':
+ pathData = PATH_ERROR;
+ break;
case 'expanded':
pathData = PATH_EXPANDED;
break;
@@ -165,6 +169,9 @@ const PATH_DELETE = `
const PATH_DOWN = 'M7.41 8.59L12 13.17l4.59-4.58L18 10l-6 6-6-6 1.41-1.41z';
+const PATH_ERROR =
+ 'M12 2C6.48 2 2 6.48 2 12s4.48 10 10 10 10-4.48 10-10S17.52 2 12 2zm1 15h-2v-2h2v2zm0-4h-2V7h2v6z';
+
const PATH_EXPANDED = 'M7 10l5 5 5-5z';
const PATH_EXPORT = 'M15.82,2.14v7H21l-9,9L3,9.18H8.18v-7ZM3,20.13H21v1.73H3Z';
diff --git a/src/devtools/views/Components/InspectedElementContext.js b/src/devtools/views/Components/InspectedElementContext.js
index 8ba1b77..0e52748 100644
--- a/src/devtools/views/Components/InspectedElementContext.js
+++ b/src/devtools/views/Components/InspectedElementContext.js
@@ -149,8 +149,10 @@ function InspectedElementContextController({ children }: Props) {
const {
canEditFunctionProps,
canEditHooks,
+ canToggleError,
canToggleSuspense,
canViewSource,
+ isErrored,
source,
type,
owners,
@@ -163,8 +165,10 @@ function InspectedElementContextController({ children }: Props) {
const inspectedElement: InspectedElementFrontend = {
canEditFunctionProps,
canEditHooks,
+ canToggleError,
canToggleSuspense,
canViewSource,
+ isErrored,
id,
source,
type,
diff --git a/src/devtools/views/Components/SelectedElement.js b/src/devtools/views/Components/SelectedElement.js
index 30e9bd5..3279e04 100644
--- a/src/devtools/views/Components/SelectedElement.js
+++ b/src/devtools/views/Components/SelectedElement.js
@@ -100,15 +100,68 @@ export default function SelectedElement(_: Props) {
(canViewElementSourceFunction === null ||
canViewElementSourceFunction(inspectedElement));
+ const isErrored =
+ inspectedElement != null &&
+ inspectedElement.isErrored != null;
+
const isSuspended =
element !== null &&
element.type === ElementTypeSuspense &&
inspectedElement != null &&
inspectedElement.state != null;
+ const canToggleError =
+ inspectedElement != null && inspectedElement.canToggleError;
const canToggleSuspense =
inspectedElement != null && inspectedElement.canToggleSuspense;
+ // TODO (error toggle) Would be nice to eventually use a two setState pattern here as well.
+ const toggleErrored = useCallback(() => {
+ let nearestErrorBoundary = null;
+ let currentElement = element;
+ while (currentElement !== null) {
+ if (currentElement.isErrorBoundary) {
+ nearestErrorBoundary = currentElement;
+ break;
+ } else if (currentElement.parentID > 0) {
+ currentElement = store.getElementByID(currentElement.parentID);
+ } else {
+ currentElement = null;
+ }
+ }
+
+ // If we didn't find an error boundary ancestor, we can't throw.
+ // Instead we can show a warning to the user.
+ if (nearestErrorBoundary === null) {
+ modalDialogDispatch({
+ type: 'SHOW',
+ content: <CannotThrowWarningMessage />,
+ });
+ } else {
+ const nearestErrorBoundaryID = nearestErrorBoundary.id;
+
+ // If we're suspending from an arbitary (non-Suspense) component, select the nearest Suspense element in the Tree.
+ // This way when the fallback UI is shown and the current element is hidden, something meaningful is selected.
+ if (nearestErrorBoundary !== element) {
+ dispatch({
+ type: 'SELECT_ELEMENT_BY_ID',
+ payload: nearestErrorBoundaryID,
+ });
+ }
+
+ const rendererID = store.getRendererIDForElement(nearestErrorBoundaryID);
+
+ // Toggle suspended
+ if (rendererID !== null) {
+ bridge.send('overrideError', {
+ id: nearestErrorBoundaryID,
+ rendererID,
+ forceError: !isErrored,
+ });
+ }
+ }
+ }, [bridge, dispatch, element, isSuspended, modalDialogDispatch, store]);
+
// TODO (suspense toggle) Would be nice to eventually use a two setState pattern here as well.
const toggleSuspended = useCallback(() => {
let nearestSuspenseElement = null;
@@ -175,6 +228,20 @@ export default function SelectedElement(_: Props) {
</div>
</div>
+ {canToggleError && (
+ <Toggle
+ className={styles.IconButton}
+ isChecked={isSuspended}
+ onChange={toggleErrored}
+ title={
+ isErrored
+ ? 'Clear the forced error'
+ : 'Force the selected component into an errored state'
+ }
+ >
+ <ButtonIcon type="error" />
+ </Toggle>
+ )}
{canToggleSuspense && (
<Toggle
className={styles.IconButton}
@@ -433,6 +500,35 @@ function OwnerView({
);
}
+function CannotThrowWarningMessage() {
+ const store = useContext(StoreContext);
+ const areClassComponentsHidden = !!store.componentFilters.find(
+ filter =>
+ filter.type === ComponentFilterElementType &&
+ filter.value === ElementTypeClass &&
+ filter.isEnabled
+ );
+
+ // Has the user filted out class nodes from the tree?
+ // If so, the selected element might actually be in an error boundary,
+ // but we have no way to know.
+ if (areClassComponentsHidden) {
+ return (
+ <div className={styles.CannotSuspendWarningMessage}>
+ Error state cannot be toggled while class components are hidden. Disable
+ the filter and try agan.
+ </div>
+ );
+ } else {
+ return (
+ <div className={styles.CannotSuspendWarningMessage}>
+ The selected element is not within an error boundary. Breaking it would
+ cause an error.
+ </div>
+ );
+ }
+}
+
function CannotSuspendWarningMessage() {
const store = useContext(StoreContext);
const areSuspenseElementsHidden = !!store.componentFilters.find(
This repository is being merged into the main React repo (github.com/facebook/react). As part of this, I am moving all issues to that repository as well and closing them here.
This issue has been relocated to: https://github.com/facebook/react/issues/16469
It would be useful to force components into an error state, in order to test error boundaries (similar to how the suspense toggle works).