From 5d39e4faff09791242cc43cbf74bdf8e019e3f6a Mon Sep 17 00:00:00 2001 From: Bao Pham Date: Sun, 23 May 2021 21:40:49 +0800 Subject: [PATCH 1/7] Devtools: add feature to trigger an error boundary --- .../__snapshots__/profilingCache-test.js.snap | 27 +++++ .../src/__tests__/inspectedElement-test.js | 58 ++++++++++ .../__tests__/inspectedElementSerializer.js | 1 + .../src/__tests__/store-test.js | 35 ++++++ .../src/backend/agent.js | 16 +++ .../src/backend/legacy/renderer.js | 10 ++ .../src/backend/renderer.js | 103 +++++++++++++++++- .../src/backend/types.js | 7 ++ .../react-devtools-shared/src/backendAPI.js | 4 + packages/react-devtools-shared/src/bridge.js | 6 + .../src/devtools/store.js | 6 + .../src/devtools/views/ButtonIcon.js | 9 +- .../Components/CannotThrowWarningMessage.js | 40 +++++++ .../views/Components/InspectedElement.js | 64 +++++++++++ .../src/devtools/views/Components/types.js | 7 ++ .../views/Profiler/CommitTreeBuilder.js | 2 + packages/react-devtools-shared/src/utils.js | 2 + .../src/app/ErrorBoundaries/index.js | 71 ++++++++++++ .../react-devtools-shell/src/app/index.js | 2 + .../src/ReactFiberBeginWork.new.js | 37 ++++++- .../src/ReactFiberBeginWork.old.js | 37 ++++++- .../src/ReactFiberReconciler.js | 4 + .../src/ReactFiberReconciler.new.js | 12 ++ .../src/ReactFiberReconciler.old.js | 12 ++ 24 files changed, 567 insertions(+), 5 deletions(-) create mode 100644 packages/react-devtools-shared/src/devtools/views/Components/CannotThrowWarningMessage.js create mode 100644 packages/react-devtools-shell/src/app/ErrorBoundaries/index.js diff --git a/packages/react-devtools-shared/src/__tests__/__snapshots__/profilingCache-test.js.snap b/packages/react-devtools-shared/src/__tests__/__snapshots__/profilingCache-test.js.snap index b27231efdffc7..57ce13bf4d80a 100644 --- a/packages/react-devtools-shared/src/__tests__/__snapshots__/profilingCache-test.js.snap +++ b/packages/react-devtools-shared/src/__tests__/__snapshots__/profilingCache-test.js.snap @@ -719,6 +719,7 @@ Object { 0, 1, 0, + 0, 4, 2, 12000, @@ -729,6 +730,7 @@ Object { 2, 2, 3, + 0, 4, 4, 0, @@ -739,6 +741,7 @@ Object { 2, 2, 4, + 0, 4, 5, 1000, @@ -749,6 +752,7 @@ Object { 2, 2, 0, + 0, 4, 6, 1000, @@ -772,6 +776,7 @@ Object { 2, 1, 2, + 0, 4, 7, 2000, @@ -1178,6 +1183,7 @@ Object { 0, 1, 0, + 0, 4, 2, 11000, @@ -1188,6 +1194,7 @@ Object { 2, 2, 3, + 0, 4, 4, 0, @@ -1198,6 +1205,7 @@ Object { 2, 2, 0, + 0, 4, 5, 1000, @@ -1221,6 +1229,7 @@ Object { 2, 1, 2, + 0, 4, 6, 1000, @@ -1256,6 +1265,7 @@ Object { 2, 1, 2, + 0, 4, 7, 2000, @@ -1455,6 +1465,7 @@ Object { 2, 1, 2, + 0, 4, 12, 2000, @@ -1647,6 +1658,7 @@ Object { 0, 1, 0, + 0, 4, 14, 11000, @@ -1657,6 +1669,7 @@ Object { 14, 2, 3, + 0, 4, 16, 0, @@ -1667,6 +1680,7 @@ Object { 14, 2, 0, + 0, 4, 17, 1000, @@ -2036,6 +2050,7 @@ Object { 2, 1, 2, + 0, 4, 12, 2000, @@ -2273,6 +2288,7 @@ Object { 0, 1, 0, + 0, 4, 14, 11000, @@ -2283,6 +2299,7 @@ Object { 14, 2, 3, + 0, 4, 16, 0, @@ -2293,6 +2310,7 @@ Object { 14, 2, 0, + 0, 4, 17, 1000, @@ -2472,6 +2490,7 @@ Object { 0, 1, 0, + 0, 4, 2, 0, @@ -2968,6 +2987,7 @@ Object { 0, 1, 0, + 0, 4, 2, 0, @@ -2978,6 +2998,7 @@ Object { 0, 2, 0, + 0, 4, 3, 0, @@ -4176,6 +4197,7 @@ Object { 0, 1, 0, + 0, 4, 2, 0, @@ -4186,6 +4208,7 @@ Object { 2, 2, 0, + 0, 4, 3, 0, @@ -4196,6 +4219,7 @@ Object { 2, 3, 0, + 0, 4, 4, 0, @@ -4206,6 +4230,7 @@ Object { 4, 4, 0, + 0, 4, 5, 0, @@ -4216,6 +4241,7 @@ Object { 2, 5, 0, + 0, 4, 6, 0, @@ -4226,6 +4252,7 @@ Object { 6, 4, 0, + 0, 4, 7, 0, diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js index 0c4fc869764e7..ca5908ef81331 100644 --- a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js +++ b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js @@ -2380,4 +2380,62 @@ describe('InspectedElement', () => { `); }); }); + + describe('error boundary', () => { + it('can toggle error', async () => { + class ErrorBoundary extends React.Component { + state = {hasError: false}; + static getDerivedStateFromError(error) { + return {hasError: true}; + } + render() { + const {hasError} = this.state; + return hasError ? 'has-error' : this.props.children; + } + } + const Example = () => 'example'; + + await utils.actAsync(() => + ReactDOM.render( + + + , + document.createElement('div'), + ), + ); + + const errorBoundaryID = ((store.getElementIDAtIndex(0): any): number); + const toggleError = async forceError => { + await withErrorsOrWarningsIgnored(['ErrorBoundary'], async () => { + await utils.actAsync(() => { + bridge.send('overrideError', { + id: errorBoundaryID, + rendererID: store.getRendererIDForElement(errorBoundaryID), + forceError, + }); + }); + }); + + TestUtilsAct(() => { + jest.runOnlyPendingTimers(); + }); + }; + + let inspectedElement = await inspectElementAtIndex(1); + expect(inspectedElement.canToggleError).toBe(true); + expect(inspectedElement.isErrored).toBe(false); + + await toggleError(true); + + inspectedElement = await inspectElementAtIndex(0); + expect(inspectedElement.canToggleError).toBe(true); + expect(inspectedElement.isErrored).toBe(true); + + await toggleError(false); + + inspectedElement = await inspectElementAtIndex(0); + expect(inspectedElement.canToggleError).toBe(true); + expect(inspectedElement.isErrored).toBe(false); + }); + }); }); diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElementSerializer.js b/packages/react-devtools-shared/src/__tests__/inspectedElementSerializer.js index db75d9e150880..d3e87c7110a0c 100644 --- a/packages/react-devtools-shared/src/__tests__/inspectedElementSerializer.js +++ b/packages/react-devtools-shared/src/__tests__/inspectedElementSerializer.js @@ -14,6 +14,7 @@ export function test(maybeInspectedElement) { hasOwnProperty('canEditFunctionProps') && hasOwnProperty('canEditHooks') && hasOwnProperty('canToggleSuspense') && + hasOwnProperty('canToggleError') && hasOwnProperty('canViewSource') ); } diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js index 04f14a41a9348..6126e26152c29 100644 --- a/packages/react-devtools-shared/src/__tests__/store-test.js +++ b/packages/react-devtools-shared/src/__tests__/store-test.js @@ -976,6 +976,41 @@ describe('Store', () => { `); }); + it('should detect if element is an error boundary', async () => { + class ErrorBoundary extends React.Component { + state = {hasError: false}; + static getDerivedStateFromError(error) { + return {hasError: true}; + } + render() { + return this.props.children; + } + } + class ClassComponent extends React.Component { + render() { + return null; + } + } + const FunctionalComponent = () => null; + + const App = () => ( + + + + + + + ); + + const container = document.createElement('div'); + + act(() => ReactDOM.render(, container)); + + expect(store.getElementAtIndex(1).isErrorBoundary).toBe(true); + expect(store.getElementAtIndex(2).isErrorBoundary).toBe(false); + expect(store.getElementAtIndex(3).isErrorBoundary).toBe(false); + }); + describe('Lazy', () => { async function fakeImport(result) { return {default: result}; diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js index 7470070455c04..4dc727a59e2f8 100644 --- a/packages/react-devtools-shared/src/backend/agent.js +++ b/packages/react-devtools-shared/src/backend/agent.js @@ -122,6 +122,12 @@ type OverrideValueAtPathParams = {| value: any, |}; +type OverrideErrorParams = {| + id: number, + rendererID: number, + forceError: boolean, +|}; + type OverrideSuspenseParams = {| id: number, rendererID: number, @@ -183,6 +189,7 @@ export default class Agent extends EventEmitter<{| bridge.addListener('getOwnersList', this.getOwnersList); bridge.addListener('inspectElement', this.inspectElement); bridge.addListener('logElementToConsole', this.logElementToConsole); + bridge.addListener('overrideError', this.overrideError); bridge.addListener('overrideSuspense', this.overrideSuspense); bridge.addListener('overrideValueAtPath', this.overrideValueAtPath); bridge.addListener('reloadAndProfile', this.reloadAndProfile); @@ -381,6 +388,15 @@ 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); + } + }; + overrideSuspense = ({ id, rendererID, diff --git a/packages/react-devtools-shared/src/backend/legacy/renderer.js b/packages/react-devtools-shared/src/backend/legacy/renderer.js index 091d755009b42..1a48bbe26c826 100644 --- a/packages/react-devtools-shared/src/backend/legacy/renderer.js +++ b/packages/react-devtools-shared/src/backend/legacy/renderer.js @@ -407,6 +407,8 @@ export function attach( pushOperation(ownerID); pushOperation(displayNameStringID); pushOperation(keyStringID); + // isErrorBoundary not supported in legacy renderer + pushOperation(0); } } @@ -800,6 +802,10 @@ export function attach( canEditFunctionPropsDeletePaths: false, canEditFunctionPropsRenamePaths: false, + // Toggle error boundary did not exist in legacy versions + canToggleError: false, + isErrored: false, + // Suspense did not exist in legacy versions canToggleSuspense: false, @@ -1016,6 +1022,9 @@ export function attach( const handlePostCommitFiberRoot = () => { throw new Error('handlePostCommitFiberRoot not supported by this renderer'); }; + const overrideError = () => { + throw new Error('overrideError not supported by this renderer'); + }; const overrideSuspense = () => { throw new Error('overrideSuspense not supported by this renderer'); }; @@ -1089,6 +1098,7 @@ export function attach( handlePostCommitFiberRoot, inspectElement, logElementToConsole, + overrideError, overrideSuspense, overrideValueAtPath, renamePath, diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index b7b8eb605615a..842ed87348ee3 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -120,6 +120,7 @@ type ReactPriorityLevelsType = {| |}; type ReactTypeOfSideEffectType = {| + DidCapture: number, NoFlags: number, PerformedWork: number, Placement: number, @@ -147,6 +148,7 @@ export function getInternalReactConstants( ReactTypeOfWork: WorkTagMap, |} { const ReactTypeOfSideEffect: ReactTypeOfSideEffectType = { + DidCapture: 0b10000000, NoFlags: 0b00, PerformedWork: 0b01, Placement: 0b10, @@ -519,7 +521,13 @@ export function attach( ReactTypeOfWork, ReactTypeOfSideEffect, } = getInternalReactConstants(version); - const {Incomplete, NoFlags, PerformedWork, Placement} = ReactTypeOfSideEffect; + const { + DidCapture, + Incomplete, + NoFlags, + PerformedWork, + Placement, + } = ReactTypeOfSideEffect; const { CacheComponent, ClassComponent, @@ -557,9 +565,13 @@ export function attach( overrideProps, overridePropsDeletePath, overridePropsRenamePath, + setErrorHandler, setSuspenseHandler, scheduleUpdate, } = renderer; + const supportsTogglingError = + typeof setErrorHandler === 'function' && + typeof scheduleUpdate === 'function'; const supportsTogglingSuspense = typeof setSuspenseHandler === 'function' && typeof scheduleUpdate === 'function'; @@ -584,6 +596,9 @@ export function attach( if (fiber != null) { fibersWithChangedErrorOrWarningCounts.add(fiber); updateMostRecentlyInspectedElementIfNecessary(id); + if (forceErrorForFiberIDs.has(id)) { + overrideError(id, false); + } } } @@ -627,6 +642,7 @@ export function attach( } function clearErrorsForFiberID(fiberID: number) { + forceErrorForFiberIDs.delete(fiberID); clearMessageCountHelper( fiberID, pendingFiberToErrorsMap, @@ -1724,6 +1740,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 = getOrGenerateFiberID(fiber); @@ -1776,6 +1809,7 @@ export function attach( pushOperation(ownerID); pushOperation(displayNameStringID); pushOperation(keyStringID); + pushOperation(isErrorBoundary(fiber) ? 1 : 0); } if (isProfilingSupported) { @@ -3062,6 +3096,9 @@ export function attach( const errors = fiberIDToErrorsMap.get(id) || new Map(); const warnings = fiberIDToWarningsMap.get(id) || new Map(); + const isErrored = + (fiber.flags & DidCapture) !== NoFlags || + forceErrorForFiberIDs.get(id) === true; return { id, @@ -3080,6 +3117,17 @@ export function attach( canEditFunctionPropsRenamePaths: typeof overridePropsRenamePath === '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.get(id) === true), + // Is this error boundary in error state. + isErrored, + canToggleSuspense: supportsTogglingSuspense && // If it's showing the real content, we can always flip fallback. @@ -3747,7 +3795,57 @@ export function attach( } // React will switch between these implementations depending on whether - // we have any manually suspended Fibers or not. + // we have any manually suspended/errored-out Fibers or not. + function shouldErrorFiberAlwaysNull() { + return null; + } + + const forceErrorForFiberIDs = new Map(); + function shouldErrorFiberAccordingToMap(fiber) { + if (typeof setErrorHandler !== 'function') { + throw new Error( + 'Expected overrideError() to not get called for earlier React versions.', + ); + } + + const id = getOrGenerateFiberID(fiber); + let status = null; + if (forceErrorForFiberIDs.has(id)) { + status = forceErrorForFiberIDs.get(id); + if (status === false) { + clearErrorsForFiberID(id); + } + + if (forceErrorForFiberIDs.size === 0) { + // Last override is gone. Switch React back to fast path. + setErrorHandler(shouldErrorFiberAlwaysNull); + } + } + return status; + } + + function overrideError(id, forceError) { + if ( + typeof setErrorHandler !== 'function' || + typeof scheduleUpdate !== 'function' + ) { + throw new Error( + 'Expected overrideError() to not get called for earlier React versions.', + ); + } + + forceErrorForFiberIDs.set(id, forceError); + + if (forceErrorForFiberIDs.size === 1) { + // First override is added. Switch React to slower path. + setErrorHandler(shouldErrorFiberAccordingToMap); + } + + const fiber = idToArbitraryFiberMap.get(id); + if (fiber != null) { + scheduleUpdate(fiber); + } + } function shouldSuspendFiberAlwaysFalse() { return false; @@ -4042,6 +4140,7 @@ export function attach( logElementToConsole, prepareViewAttributeSource, prepareViewElementSource, + overrideError, overrideSuspense, overrideValueAtPath, renamePath, diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index 342931c250eab..94e4776ee33cf 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -142,6 +142,8 @@ export type ReactRenderer = { ComponentTree?: any, // Present for React DOM v12 (possibly earlier) through v15. Mount?: any, + // Only injected by React v17.0.3+ in DEV mode + setErrorHandler?: ?(shouldError: (fiber: Object) => ?boolean) => void, ... }; @@ -224,6 +226,10 @@ export type InspectedElement = {| canEditFunctionPropsDeletePaths: boolean, canEditFunctionPropsRenamePaths: boolean, + // Is this Error, and can its value be overridden now? + canToggleError: boolean, + isErrored: boolean, + // Is this Suspense, and can its value be overridden now? canToggleSuspense: boolean, @@ -332,6 +338,7 @@ export type RendererInterface = { inspectedPaths: Object, ) => InspectedElementPayload, logElementToConsole: (id: number) => void, + overrideError: (id: number, forceError: boolean) => void, overrideSuspense: (id: number, forceFallback: boolean) => void, overrideValueAtPath: ( type: Type, diff --git a/packages/react-devtools-shared/src/backendAPI.js b/packages/react-devtools-shared/src/backendAPI.js index a4b870ea8f9b1..9f72bb9c1d914 100644 --- a/packages/react-devtools-shared/src/backendAPI.js +++ b/packages/react-devtools-shared/src/backendAPI.js @@ -190,6 +190,8 @@ export function convertInspectedElementBackendToFrontend( canEditHooks, canEditHooksAndDeletePaths, canEditHooksAndRenamePaths, + canToggleError, + isErrored, canToggleSuspense, canViewSource, hasLegacyContext, @@ -216,6 +218,8 @@ export function convertInspectedElementBackendToFrontend( canEditHooks, canEditHooksAndDeletePaths, canEditHooksAndRenamePaths, + canToggleError, + isErrored, canToggleSuspense, canViewSource, hasLegacyContext, diff --git a/packages/react-devtools-shared/src/bridge.js b/packages/react-devtools-shared/src/bridge.js index 5b34a2321253c..5bf8a5f127c9c 100644 --- a/packages/react-devtools-shared/src/bridge.js +++ b/packages/react-devtools-shared/src/bridge.js @@ -115,6 +115,11 @@ type OverrideValueAtPath = {| value: any, |}; +type OverrideError = {| + ...ElementAndRendererID, + forceError: boolean, +|}; + type OverrideSuspense = {| ...ElementAndRendererID, forceFallback: boolean, @@ -201,6 +206,7 @@ type FrontendEvents = {| highlightNativeElement: [HighlightElementInDOM], inspectElement: [InspectElementParams], logElementToConsole: [ElementAndRendererID], + overrideError: [OverrideError], overrideSuspense: [OverrideSuspense], overrideValueAtPath: [OverrideValueAtPath], profilingData: [ProfilingDataBackend], diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js index 0d2ca5faa88ed..0bfd1935e3be4 100644 --- a/packages/react-devtools-shared/src/devtools/store.js +++ b/packages/react-devtools-shared/src/devtools/store.js @@ -837,6 +837,7 @@ export default class Store extends EventEmitter<{| ); } + let isErrorBoundary: boolean = false; let ownerID: number = 0; let parentID: number = ((null: any): number); if (type === ElementTypeRoot) { @@ -864,6 +865,7 @@ export default class Store extends EventEmitter<{| hocDisplayNames: null, id, isCollapsed: false, // Never collapse roots; it would hide the entire tree. + isErrorBoundary: false, key: null, ownerID: 0, parentID: 0, @@ -887,6 +889,9 @@ export default class Store extends EventEmitter<{| const key = stringTable[keyStringID]; i++; + isErrorBoundary = ((operations[i]: any): number) > 0; + i++; + if (__DEBUG__) { debug( 'Add', @@ -919,6 +924,7 @@ export default class Store extends EventEmitter<{| hocDisplayNames, id, isCollapsed: this._collapseNodesByDefault, + isErrorBoundary, key, ownerID, parentID: parentElement.id, diff --git a/packages/react-devtools-shared/src/devtools/views/ButtonIcon.js b/packages/react-devtools-shared/src/devtools/views/ButtonIcon.js index 0c7fe00b42aac..fffd0aec4cbc6 100644 --- a/packages/react-devtools-shared/src/devtools/views/ButtonIcon.js +++ b/packages/react-devtools-shared/src/devtools/views/ButtonIcon.js @@ -32,6 +32,7 @@ export type IconType = | 'save' | 'search' | 'settings' + | 'error' | 'suspend' | 'undo' | 'up' @@ -109,6 +110,9 @@ export default function ButtonIcon({className = '', type}: Props) { case 'settings': pathData = PATH_SETTINGS; break; + case 'error': + pathData = PATH_ERROR; + break; case 'suspend': pathData = PATH_SUSPEND; break; @@ -187,7 +191,7 @@ const PATH_LOG_DATA = ` `; const PATH_MORE = ` - M12 8c1.1 0 2-.9 2-2s-.9-2-2-2-2 .9-2 2 .9 2 2 2zm0 2c-1.1 0-2 .9-2 2s.9 + M12 8c1.1 0 2-.9 2-2s-.9-2-2-2-2 .9-2 2 .9 2 2 2zm0 2c-1.1 0-2 .9-2 2s.9 2 2 2 2-.9 2-2-.9-2-2-2zm0 6c-1.1 0-2 .9-2 2s.9 2 2 2 2-.9 2-2-.9-2-2-2z `; @@ -223,6 +227,9 @@ const PATH_SETTINGS = ` 3.5-3.5 3.5 1.57 3.5 3.5-1.57 3.5-3.5 3.5z `; +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_SUSPEND = ` M15 1H9v2h6V1zm-4 13h2V8h-2v6zm8.03-6.61l1.42-1.42c-.43-.51-.9-.99-1.41-1.41l-1.42 1.42C16.07 4.74 14.12 4 12 4c-4.97 0-9 4.03-9 9s4.02 9 9 9 9-4.03 9-9c0-2.12-.74-4.07-1.97-5.61zM12 20c-3.87 0-7-3.13-7-7s3.13-7 7-7 7 3.13 7 7-3.13 7-7 7z diff --git a/packages/react-devtools-shared/src/devtools/views/Components/CannotThrowWarningMessage.js b/packages/react-devtools-shared/src/devtools/views/Components/CannotThrowWarningMessage.js new file mode 100644 index 0000000000000..f1db2dd03533b --- /dev/null +++ b/packages/react-devtools-shared/src/devtools/views/Components/CannotThrowWarningMessage.js @@ -0,0 +1,40 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +import * as React from 'react'; +import {useContext} from 'react'; +import {StoreContext} from '../context'; +import { + ComponentFilterElementType, + ElementTypeClass, +} from 'react-devtools-shared/src/types'; + +export default function CannotThrowWarningMessage() { + const store = useContext(StoreContext); + const areClassComponentsHidden = !!store.componentFilters.find( + filter => + filter.type === ComponentFilterElementType && + filter.value === ElementTypeClass && + filter.isEnabled, + ); + + // Has the user filtered 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 ( +
+ Error state cannot be toggled while class components are hidden. Disable + the filter and try again. +
+ ); + } else { + return
The selected element is not within an error boundary.
; + } +} diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js index e7cda475052ec..cbd3c5b866746 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js @@ -17,6 +17,7 @@ import {ModalDialogContext} from '../ModalDialog'; import ViewElementSourceContext from './ViewElementSourceContext'; import Toggle from '../Toggle'; import {ElementTypeSuspense} from 'react-devtools-shared/src/types'; +import CannotThrowWarningMessage from './CannotThrowWarningMessage'; import CannotSuspendWarningMessage from './CannotSuspendWarningMessage'; import InspectedElementView from './InspectedElementView'; import {InspectedElementContext} from './InspectedElementContext'; @@ -92,15 +93,65 @@ export default function InspectedElementWrapper(_: Props) { (canViewElementSourceFunction === null || canViewElementSourceFunction(inspectedElement)); + const isErrored = inspectedElement != null && inspectedElement.isErrored; + const isSuspended = element !== null && element.type === ElementTypeSuspense && inspectedElement != null && inspectedElement.state != null; + const canToggleError = + inspectedElement != null && inspectedElement.canToggleError; + const canToggleSuspense = inspectedElement != null && inspectedElement.canToggleSuspense; + 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({ + id: 'InspectedElement', + type: 'SHOW', + content: , + }); + } else { + const nearestErrorBoundaryID = nearestErrorBoundary.id; + + if (nearestErrorBoundary !== element) { + dispatch({ + type: 'SELECT_ELEMENT_BY_ID', + payload: nearestErrorBoundaryID, + }); + } + + const rendererID = store.getRendererIDForElement(nearestErrorBoundaryID); + + // Toggle error. + if (rendererID !== null) { + bridge.send('overrideError', { + id: nearestErrorBoundaryID, + rendererID, + forceError: !isErrored, + }); + } + } + }, [bridge, dispatch, element, isErrored, modalDialogDispatch, store]); + // TODO (suspense toggle) Would be nice to eventually use a two setState pattern here as well. const toggleSuspended = useCallback(() => { let nearestSuspenseElement = null; @@ -177,6 +228,19 @@ export default function InspectedElementWrapper(_: Props) { + {canToggleError && ( + + + + )} {canToggleSuspense && ( ) { i++; // key + i++; // isErrorBoundary + logs.push( `Add node ${id} (${displayName || 'null'}) as child of ${parentID}`, ); diff --git a/packages/react-devtools-shell/src/app/ErrorBoundaries/index.js b/packages/react-devtools-shell/src/app/ErrorBoundaries/index.js new file mode 100644 index 0000000000000..1a4fc7927c06c --- /dev/null +++ b/packages/react-devtools-shell/src/app/ErrorBoundaries/index.js @@ -0,0 +1,71 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +import * as React from 'react'; +import {Fragment} from 'react'; + +class ErrorBoundary extends React.Component { + state = {hasError: false}; + + static getDerivedStateFromError(error) { + return {hasError: true}; + } + + render() { + const {hasError} = this.state; + if (hasError) { + return ( +
+ An error was thrown. +
+ ); + } + + const {children} = this.props; + return ( +
+ {children} +
+ ); + } +} + +function Component({label}) { + return
{label}
; +} + +export default function ErrorBoundaries() { + return ( + +

Nested error boundaries demo

+ + + + + + + + + + + ); +} diff --git a/packages/react-devtools-shell/src/app/index.js b/packages/react-devtools-shell/src/app/index.js index 5764130075aec..d00ffde4a5ded 100644 --- a/packages/react-devtools-shell/src/app/index.js +++ b/packages/react-devtools-shell/src/app/index.js @@ -17,6 +17,7 @@ import InspectableElements from './InspectableElements'; import ReactNativeWeb from './ReactNativeWeb'; import ToDoList from './ToDoList'; import Toggle from './Toggle'; +import ErrorBoundaries from './ErrorBoundaries'; import SuspenseTree from './SuspenseTree'; import {ignoreErrors, ignoreWarnings} from './console'; @@ -54,6 +55,7 @@ function mountTestApp() { mountHelper(InlineWarnings); mountHelper(ReactNativeWeb); mountHelper(Toggle); + mountHelper(ErrorBoundaries); mountHelper(SuspenseTree); mountHelper(DeeplyNestedComponents); mountHelper(Iframe); diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js index 6f1f7953b257a..bf48a1eb47a1d 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.new.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.new.js @@ -70,6 +70,7 @@ import { ChildDeletion, ForceUpdateForLegacySuspense, StaticMask, + ShouldCapture, } from './ReactFiberFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import { @@ -111,6 +112,7 @@ import { processUpdateQueue, cloneUpdateQueue, initializeUpdateQueue, + enqueueCapturedUpdate, } from './ReactUpdateQueue.new'; import { NoLane, @@ -125,6 +127,7 @@ import { removeLanes, mergeLanes, getBumpedLaneForHydration, + pickArbitraryLane, } from './ReactFiberLane.new'; import { ConcurrentMode, @@ -141,7 +144,7 @@ import { isPrimaryRenderer, } from './ReactFiberHostConfig'; import type {SuspenseInstance} from './ReactFiberHostConfig'; -import {shouldSuspend} from './ReactFiberReconciler'; +import {shouldError, shouldSuspend} from './ReactFiberReconciler'; import {pushHostContext, pushHostContainer} from './ReactFiberHostContext.new'; import { suspenseStackCursor, @@ -219,6 +222,8 @@ import { restoreSpawnedCachePool, getOffscreenDeferredCachePool, } from './ReactFiberCacheComponent.new'; +import {createCapturedValue} from './ReactCapturedValue'; +import {createClassErrorUpdate} from './ReactFiberThrow.new'; import is from 'shared/objectIs'; import {disableLogs, reenableLogs} from 'shared/ConsolePatchingDev'; @@ -947,6 +952,36 @@ function updateClassComponent( renderLanes: Lanes, ) { if (__DEV__) { + // This is used by DevTools to force a boundary to error. + switch (shouldError(workInProgress)) { + case false: { + const instance = workInProgress.stateNode; + const ctor = workInProgress.type; + const tempInstance = new ctor( + workInProgress.memoizedProps, + instance.context, + ); + const state = tempInstance.state; + instance.updater.enqueueSetState(instance, state, null); + break; + } + case true: { + workInProgress.flags |= DidCapture; + workInProgress.flags |= ShouldCapture; + const error = new Error('Simulated error coming from DevTools'); + const lane = pickArbitraryLane(renderLanes); + workInProgress.lanes = mergeLanes(workInProgress.lanes, lane); + // Schedule the error boundary to re-render using updated state + const update = createClassErrorUpdate( + workInProgress, + createCapturedValue(error, workInProgress), + lane, + ); + enqueueCapturedUpdate(workInProgress, update); + break; + } + } + if (workInProgress.type !== workInProgress.elementType) { // Lazy component props can't be validated in createElement // because they're only guaranteed to be resolved here. diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.old.js b/packages/react-reconciler/src/ReactFiberBeginWork.old.js index 8bac08c0ff6f1..cfb7e14beefb7 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.old.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.old.js @@ -70,6 +70,7 @@ import { ChildDeletion, ForceUpdateForLegacySuspense, StaticMask, + ShouldCapture, } from './ReactFiberFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import { @@ -111,6 +112,7 @@ import { processUpdateQueue, cloneUpdateQueue, initializeUpdateQueue, + enqueueCapturedUpdate, } from './ReactUpdateQueue.old'; import { NoLane, @@ -125,6 +127,7 @@ import { removeLanes, mergeLanes, getBumpedLaneForHydration, + pickArbitraryLane, } from './ReactFiberLane.old'; import { ConcurrentMode, @@ -141,7 +144,7 @@ import { isPrimaryRenderer, } from './ReactFiberHostConfig'; import type {SuspenseInstance} from './ReactFiberHostConfig'; -import {shouldSuspend} from './ReactFiberReconciler'; +import {shouldError, shouldSuspend} from './ReactFiberReconciler'; import {pushHostContext, pushHostContainer} from './ReactFiberHostContext.old'; import { suspenseStackCursor, @@ -219,6 +222,8 @@ import { restoreSpawnedCachePool, getOffscreenDeferredCachePool, } from './ReactFiberCacheComponent.old'; +import {createCapturedValue} from './ReactCapturedValue'; +import {createClassErrorUpdate} from './ReactFiberThrow.old'; import is from 'shared/objectIs'; import {disableLogs, reenableLogs} from 'shared/ConsolePatchingDev'; @@ -947,6 +952,36 @@ function updateClassComponent( renderLanes: Lanes, ) { if (__DEV__) { + // This is used by DevTools to force a boundary to error. + switch (shouldError(workInProgress)) { + case false: { + const instance = workInProgress.stateNode; + const ctor = workInProgress.type; + const tempInstance = new ctor( + workInProgress.memoizedProps, + instance.context, + ); + const state = tempInstance.state; + instance.updater.enqueueSetState(instance, state, null); + break; + } + case true: { + workInProgress.flags |= DidCapture; + workInProgress.flags |= ShouldCapture; + const error = new Error('Simulated error coming from DevTools'); + const lane = pickArbitraryLane(renderLanes); + workInProgress.lanes = mergeLanes(workInProgress.lanes, lane); + // Schedule the error boundary to re-render using updated state + const update = createClassErrorUpdate( + workInProgress, + createCapturedValue(error, workInProgress), + lane, + ); + enqueueCapturedUpdate(workInProgress, update); + break; + } + } + if (workInProgress.type !== workInProgress.elementType) { // Lazy component props can't be validated in createElement // because they're only guaranteed to be resolved here. diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index e789b58b61c11..a78960f426829 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -75,6 +75,7 @@ import { findHostInstance as findHostInstance_new, findHostInstanceWithWarning as findHostInstanceWithWarning_new, findHostInstanceWithNoPortals as findHostInstanceWithNoPortals_new, + shouldError as shouldError_new, shouldSuspend as shouldSuspend_new, injectIntoDevTools as injectIntoDevTools_new, act as act_new, @@ -155,6 +156,9 @@ export const findHostInstanceWithWarning = enableNewReconciler export const findHostInstanceWithNoPortals = enableNewReconciler ? findHostInstanceWithNoPortals_new : findHostInstanceWithNoPortals_old; +export const shouldError = enableNewReconciler + ? shouldError_new + : shouldError_new; export const shouldSuspend = enableNewReconciler ? shouldSuspend_new : shouldSuspend_old; diff --git a/packages/react-reconciler/src/ReactFiberReconciler.new.js b/packages/react-reconciler/src/ReactFiberReconciler.new.js index c1e9778160a7c..c53fabbb8e4ae 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.new.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.new.js @@ -463,6 +463,12 @@ export function findHostInstanceWithNoPortals( return hostFiber.stateNode; } +let shouldErrorImpl = fiber => null; + +export function shouldError(fiber: Fiber): ?boolean { + return shouldErrorImpl(fiber); +} + let shouldSuspendImpl = fiber => false; export function shouldSuspend(fiber: Fiber): boolean { @@ -476,6 +482,7 @@ let overrideProps = null; let overridePropsDeletePath = null; let overridePropsRenamePath = null; let scheduleUpdate = null; +let setErrorHandler = null; let setSuspenseHandler = null; if (__DEV__) { @@ -690,6 +697,10 @@ if (__DEV__) { scheduleUpdateOnFiber(fiber, SyncLane, NoTimestamp); }; + setErrorHandler = (newShouldErrorImpl: Fiber => ?boolean) => { + shouldErrorImpl = newShouldErrorImpl; + }; + setSuspenseHandler = (newShouldSuspendImpl: Fiber => boolean) => { shouldSuspendImpl = newShouldSuspendImpl; }; @@ -728,6 +739,7 @@ export function injectIntoDevTools(devToolsConfig: DevToolsConfig): boolean { overrideProps, overridePropsDeletePath, overridePropsRenamePath, + setErrorHandler, setSuspenseHandler, scheduleUpdate, currentDispatcherRef: ReactCurrentDispatcher, diff --git a/packages/react-reconciler/src/ReactFiberReconciler.old.js b/packages/react-reconciler/src/ReactFiberReconciler.old.js index 47ced00130310..73d412bfcd1ae 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.old.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.old.js @@ -463,6 +463,12 @@ export function findHostInstanceWithNoPortals( return hostFiber.stateNode; } +let shouldErrorImpl = fiber => null; + +export function shouldError(fiber: Fiber): ?boolean { + return shouldErrorImpl(fiber); +} + let shouldSuspendImpl = fiber => false; export function shouldSuspend(fiber: Fiber): boolean { @@ -476,6 +482,7 @@ let overrideProps = null; let overridePropsDeletePath = null; let overridePropsRenamePath = null; let scheduleUpdate = null; +let setErrorHandler = null; let setSuspenseHandler = null; if (__DEV__) { @@ -690,6 +697,10 @@ if (__DEV__) { scheduleUpdateOnFiber(fiber, SyncLane, NoTimestamp); }; + setErrorHandler = (newShouldErrorImpl: Fiber => ?boolean) => { + shouldErrorImpl = newShouldErrorImpl; + }; + setSuspenseHandler = (newShouldSuspendImpl: Fiber => boolean) => { shouldSuspendImpl = newShouldSuspendImpl; }; @@ -728,6 +739,7 @@ export function injectIntoDevTools(devToolsConfig: DevToolsConfig): boolean { overrideProps, overridePropsDeletePath, overridePropsRenamePath, + setErrorHandler, setSuspenseHandler, scheduleUpdate, currentDispatcherRef: ReactCurrentDispatcher, From 36827a42019d55f2ac5d71748c4e446b85f8cb8f Mon Sep 17 00:00:00 2001 From: Bao Pham Date: Mon, 31 May 2021 18:36:52 +0800 Subject: [PATCH 2/7] Fix not importing from ReactFiberReconciler.old --- packages/react-reconciler/src/ReactFiberReconciler.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index a78960f426829..ff68770f09b06 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -35,6 +35,7 @@ import { findHostInstance as findHostInstance_old, findHostInstanceWithWarning as findHostInstanceWithWarning_old, findHostInstanceWithNoPortals as findHostInstanceWithNoPortals_old, + shouldError as shouldError_old, shouldSuspend as shouldSuspend_old, injectIntoDevTools as injectIntoDevTools_old, act as act_old, @@ -158,7 +159,7 @@ export const findHostInstanceWithNoPortals = enableNewReconciler : findHostInstanceWithNoPortals_old; export const shouldError = enableNewReconciler ? shouldError_new - : shouldError_new; + : shouldError_old; export const shouldSuspend = enableNewReconciler ? shouldSuspend_new : shouldSuspend_old; From 958b787630c1986fd0f7689a87194c3e55a04ac6 Mon Sep 17 00:00:00 2001 From: Bao Pham Date: Wed, 2 Jun 2021 22:45:15 +0800 Subject: [PATCH 3/7] Avoid eagerly detecting whether component is error boundary and separate the concepts between forced error states and console errors With this approach, we can get rid of the warning dialog --- .../__snapshots__/profilingCache-test.js.snap | 27 ------ .../src/__tests__/inspectedElement-test.js | 31 ++++++- .../src/__tests__/store-test.js | 35 ------- .../src/backend/legacy/renderer.js | 3 +- .../src/backend/renderer.js | 92 ++++++++++++------- .../src/backend/types.js | 1 + .../react-devtools-shared/src/backendAPI.js | 2 + .../src/devtools/store.js | 6 -- .../Components/CannotThrowWarningMessage.js | 40 -------- .../views/Components/InspectedElement.js | 57 ++++-------- .../src/devtools/views/Components/types.js | 4 +- .../views/Profiler/CommitTreeBuilder.js | 2 - packages/react-devtools-shared/src/utils.js | 2 - 13 files changed, 113 insertions(+), 189 deletions(-) delete mode 100644 packages/react-devtools-shared/src/devtools/views/Components/CannotThrowWarningMessage.js diff --git a/packages/react-devtools-shared/src/__tests__/__snapshots__/profilingCache-test.js.snap b/packages/react-devtools-shared/src/__tests__/__snapshots__/profilingCache-test.js.snap index 57ce13bf4d80a..b27231efdffc7 100644 --- a/packages/react-devtools-shared/src/__tests__/__snapshots__/profilingCache-test.js.snap +++ b/packages/react-devtools-shared/src/__tests__/__snapshots__/profilingCache-test.js.snap @@ -719,7 +719,6 @@ Object { 0, 1, 0, - 0, 4, 2, 12000, @@ -730,7 +729,6 @@ Object { 2, 2, 3, - 0, 4, 4, 0, @@ -741,7 +739,6 @@ Object { 2, 2, 4, - 0, 4, 5, 1000, @@ -752,7 +749,6 @@ Object { 2, 2, 0, - 0, 4, 6, 1000, @@ -776,7 +772,6 @@ Object { 2, 1, 2, - 0, 4, 7, 2000, @@ -1183,7 +1178,6 @@ Object { 0, 1, 0, - 0, 4, 2, 11000, @@ -1194,7 +1188,6 @@ Object { 2, 2, 3, - 0, 4, 4, 0, @@ -1205,7 +1198,6 @@ Object { 2, 2, 0, - 0, 4, 5, 1000, @@ -1229,7 +1221,6 @@ Object { 2, 1, 2, - 0, 4, 6, 1000, @@ -1265,7 +1256,6 @@ Object { 2, 1, 2, - 0, 4, 7, 2000, @@ -1465,7 +1455,6 @@ Object { 2, 1, 2, - 0, 4, 12, 2000, @@ -1658,7 +1647,6 @@ Object { 0, 1, 0, - 0, 4, 14, 11000, @@ -1669,7 +1657,6 @@ Object { 14, 2, 3, - 0, 4, 16, 0, @@ -1680,7 +1667,6 @@ Object { 14, 2, 0, - 0, 4, 17, 1000, @@ -2050,7 +2036,6 @@ Object { 2, 1, 2, - 0, 4, 12, 2000, @@ -2288,7 +2273,6 @@ Object { 0, 1, 0, - 0, 4, 14, 11000, @@ -2299,7 +2283,6 @@ Object { 14, 2, 3, - 0, 4, 16, 0, @@ -2310,7 +2293,6 @@ Object { 14, 2, 0, - 0, 4, 17, 1000, @@ -2490,7 +2472,6 @@ Object { 0, 1, 0, - 0, 4, 2, 0, @@ -2987,7 +2968,6 @@ Object { 0, 1, 0, - 0, 4, 2, 0, @@ -2998,7 +2978,6 @@ Object { 0, 2, 0, - 0, 4, 3, 0, @@ -4197,7 +4176,6 @@ Object { 0, 1, 0, - 0, 4, 2, 0, @@ -4208,7 +4186,6 @@ Object { 2, 2, 0, - 0, 4, 3, 0, @@ -4219,7 +4196,6 @@ Object { 2, 3, 0, - 0, 4, 4, 0, @@ -4230,7 +4206,6 @@ Object { 4, 4, 0, - 0, 4, 5, 0, @@ -4241,7 +4216,6 @@ Object { 2, 5, 0, - 0, 4, 6, 0, @@ -4252,7 +4226,6 @@ Object { 6, 4, 0, - 0, 4, 7, 0, diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js index ca5908ef81331..2d8dc081722af 100644 --- a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js +++ b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js @@ -2405,6 +2405,14 @@ describe('InspectedElement', () => { ); const errorBoundaryID = ((store.getElementIDAtIndex(0): any): number); + const inspect = index => { + // HACK: Recreate TestRenderer instance so we can inspect different + // elements + testRendererInstance = TestRenderer.create(null, { + unstable_isConcurrent: true, + }); + return inspectElementAtIndex(index); + }; const toggleError = async forceError => { await withErrorsOrWarningsIgnored(['ErrorBoundary'], async () => { await utils.actAsync(() => { @@ -2421,21 +2429,38 @@ describe('InspectedElement', () => { }); }; - let inspectedElement = await inspectElementAtIndex(1); + // Inspect and see that we cannot toggle error state + // on error boundary itself + let inspectedElement = await inspect(0); + expect(inspectedElement.canToggleError).toBe(false); + expect(inspectedElement.errorBoundaryID).toBe(null); + + // Inspect + inspectedElement = await inspect(1); expect(inspectedElement.canToggleError).toBe(true); expect(inspectedElement.isErrored).toBe(false); + expect(inspectedElement.errorBoundaryID).toBe(errorBoundaryID); + // now force error state on await toggleError(true); - inspectedElement = await inspectElementAtIndex(0); + // we are in error state now, won't show up + expect(store.getElementIDAtIndex(1)).toBe(null); + + // Inpsect to toggle off the error state + inspectedElement = await inspect(0); expect(inspectedElement.canToggleError).toBe(true); expect(inspectedElement.isErrored).toBe(true); + // its error boundary ID is itself because it's caught the error + expect(inspectedElement.errorBoundaryID).toBe(errorBoundaryID); await toggleError(false); - inspectedElement = await inspectElementAtIndex(0); + // We can now inspect with ability to toggle again + inspectedElement = await inspect(1); expect(inspectedElement.canToggleError).toBe(true); expect(inspectedElement.isErrored).toBe(false); + expect(inspectedElement.errorBoundaryID).toBe(errorBoundaryID); }); }); }); diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js index 6126e26152c29..04f14a41a9348 100644 --- a/packages/react-devtools-shared/src/__tests__/store-test.js +++ b/packages/react-devtools-shared/src/__tests__/store-test.js @@ -976,41 +976,6 @@ describe('Store', () => { `); }); - it('should detect if element is an error boundary', async () => { - class ErrorBoundary extends React.Component { - state = {hasError: false}; - static getDerivedStateFromError(error) { - return {hasError: true}; - } - render() { - return this.props.children; - } - } - class ClassComponent extends React.Component { - render() { - return null; - } - } - const FunctionalComponent = () => null; - - const App = () => ( - - - - - - - ); - - const container = document.createElement('div'); - - act(() => ReactDOM.render(, container)); - - expect(store.getElementAtIndex(1).isErrorBoundary).toBe(true); - expect(store.getElementAtIndex(2).isErrorBoundary).toBe(false); - expect(store.getElementAtIndex(3).isErrorBoundary).toBe(false); - }); - describe('Lazy', () => { async function fakeImport(result) { return {default: result}; diff --git a/packages/react-devtools-shared/src/backend/legacy/renderer.js b/packages/react-devtools-shared/src/backend/legacy/renderer.js index 1a48bbe26c826..36e6459b128c0 100644 --- a/packages/react-devtools-shared/src/backend/legacy/renderer.js +++ b/packages/react-devtools-shared/src/backend/legacy/renderer.js @@ -407,8 +407,6 @@ export function attach( pushOperation(ownerID); pushOperation(displayNameStringID); pushOperation(keyStringID); - // isErrorBoundary not supported in legacy renderer - pushOperation(0); } } @@ -805,6 +803,7 @@ export function attach( // Toggle error boundary did not exist in legacy versions canToggleError: false, isErrored: false, + errorBoundaryID: null, // Suspense did not exist in legacy versions canToggleSuspense: false, diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index 842ed87348ee3..90a5b36e2540e 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -596,9 +596,6 @@ export function attach( if (fiber != null) { fibersWithChangedErrorOrWarningCounts.add(fiber); updateMostRecentlyInspectedElementIfNecessary(id); - if (forceErrorForFiberIDs.has(id)) { - overrideError(id, false); - } } } @@ -642,7 +639,6 @@ export function attach( } function clearErrorsForFiberID(fiberID: number) { - forceErrorForFiberIDs.delete(fiberID); clearMessageCountHelper( fiberID, pendingFiberToErrorsMap, @@ -675,6 +671,13 @@ export function attach( type: 'error' | 'warn', args: $ReadOnlyArray, ): void { + if (type === 'error') { + const maybeID = getFiberIDUnsafe(fiber); + // if this is an error simulated by us to trigger error boundary, ignore + if (maybeID != null && forceErrorForFiberIDs.get(maybeID) === true) { + return; + } + } const message = format(...args); if (__DEBUG__) { debug('onErrorOrWarning', fiber, null, `${type}: "${message}"`); @@ -1149,6 +1152,13 @@ export function attach( if (alternate !== null) { fiberToIDMap.delete(alternate); } + + if (forceErrorForFiberIDs.has(fiberID)) { + forceErrorForFiberIDs.delete(fiberID); + if (forceErrorForFiberIDs.size === 0 && setErrorHandler != null) { + setErrorHandler(shouldErrorFiberAlwaysNull); + } + } }); untrackFibersSet.clear(); } @@ -1740,23 +1750,6 @@ 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 = getOrGenerateFiberID(fiber); @@ -1809,7 +1802,6 @@ export function attach( pushOperation(ownerID); pushOperation(displayNameStringID); pushOperation(keyStringID); - pushOperation(isErrorBoundary(fiber) ? 1 : 0); } if (isProfilingSupported) { @@ -2943,6 +2935,34 @@ export function attach( return {instance, style}; } + 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 getNearestErrorBoundaryID(fiber: Fiber): number | null { + let parent = fiber.return; + while (parent !== null) { + if (isErrorBoundary(parent)) { + return getFiberIDUnsafe(parent); + } + parent = parent.return; + } + return null; + } + function inspectElementRaw(id: number): InspectedElement | null { const fiber = findCurrentFiberUsingSlowPathById(id); if (fiber == null) { @@ -3096,9 +3116,24 @@ export function attach( const errors = fiberIDToErrorsMap.get(id) || new Map(); const warnings = fiberIDToWarningsMap.get(id) || new Map(); + const isErrored = (fiber.flags & DidCapture) !== NoFlags || forceErrorForFiberIDs.get(id) === true; + const nearestErrorBoundaryID = getNearestErrorBoundaryID(fiber); + + let errorBoundaryID; + if (isErrorBoundary(fiber)) { + // if the current inspected element is an error boundary, + // either that we want to use it to toggle off error state + // or that we allow to force error state on it if it's within another + // error boundary + errorBoundaryID = isErrored + ? id + : nearestErrorBoundaryID; + } else { + errorBoundaryID = nearestErrorBoundaryID; + } return { id, @@ -3117,16 +3152,10 @@ export function attach( canEditFunctionPropsRenamePaths: typeof overridePropsRenamePath === '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.get(id) === true), + canToggleError: supportsTogglingError && errorBoundaryID != null, // Is this error boundary in error state. isErrored, + errorBoundaryID, canToggleSuspense: supportsTogglingSuspense && @@ -3809,11 +3838,12 @@ export function attach( } const id = getOrGenerateFiberID(fiber); + let status = null; if (forceErrorForFiberIDs.has(id)) { status = forceErrorForFiberIDs.get(id); if (status === false) { - clearErrorsForFiberID(id); + forceErrorForFiberIDs.delete(id); } if (forceErrorForFiberIDs.size === 0) { diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index 94e4776ee33cf..658f57f959804 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -229,6 +229,7 @@ export type InspectedElement = {| // Is this Error, and can its value be overridden now? canToggleError: boolean, isErrored: boolean, + errorBoundaryID: ?number, // Is this Suspense, and can its value be overridden now? canToggleSuspense: boolean, diff --git a/packages/react-devtools-shared/src/backendAPI.js b/packages/react-devtools-shared/src/backendAPI.js index 9f72bb9c1d914..d1e360ec5485d 100644 --- a/packages/react-devtools-shared/src/backendAPI.js +++ b/packages/react-devtools-shared/src/backendAPI.js @@ -192,6 +192,7 @@ export function convertInspectedElementBackendToFrontend( canEditHooksAndRenamePaths, canToggleError, isErrored, + errorBoundaryID, canToggleSuspense, canViewSource, hasLegacyContext, @@ -220,6 +221,7 @@ export function convertInspectedElementBackendToFrontend( canEditHooksAndRenamePaths, canToggleError, isErrored, + errorBoundaryID, canToggleSuspense, canViewSource, hasLegacyContext, diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js index 0bfd1935e3be4..0d2ca5faa88ed 100644 --- a/packages/react-devtools-shared/src/devtools/store.js +++ b/packages/react-devtools-shared/src/devtools/store.js @@ -837,7 +837,6 @@ export default class Store extends EventEmitter<{| ); } - let isErrorBoundary: boolean = false; let ownerID: number = 0; let parentID: number = ((null: any): number); if (type === ElementTypeRoot) { @@ -865,7 +864,6 @@ export default class Store extends EventEmitter<{| hocDisplayNames: null, id, isCollapsed: false, // Never collapse roots; it would hide the entire tree. - isErrorBoundary: false, key: null, ownerID: 0, parentID: 0, @@ -889,9 +887,6 @@ export default class Store extends EventEmitter<{| const key = stringTable[keyStringID]; i++; - isErrorBoundary = ((operations[i]: any): number) > 0; - i++; - if (__DEBUG__) { debug( 'Add', @@ -924,7 +919,6 @@ export default class Store extends EventEmitter<{| hocDisplayNames, id, isCollapsed: this._collapseNodesByDefault, - isErrorBoundary, key, ownerID, parentID: parentElement.id, diff --git a/packages/react-devtools-shared/src/devtools/views/Components/CannotThrowWarningMessage.js b/packages/react-devtools-shared/src/devtools/views/Components/CannotThrowWarningMessage.js deleted file mode 100644 index f1db2dd03533b..0000000000000 --- a/packages/react-devtools-shared/src/devtools/views/Components/CannotThrowWarningMessage.js +++ /dev/null @@ -1,40 +0,0 @@ -/** - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - * - * @flow - */ - -import * as React from 'react'; -import {useContext} from 'react'; -import {StoreContext} from '../context'; -import { - ComponentFilterElementType, - ElementTypeClass, -} from 'react-devtools-shared/src/types'; - -export default function CannotThrowWarningMessage() { - const store = useContext(StoreContext); - const areClassComponentsHidden = !!store.componentFilters.find( - filter => - filter.type === ComponentFilterElementType && - filter.value === ElementTypeClass && - filter.isEnabled, - ); - - // Has the user filtered 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 ( -
- Error state cannot be toggled while class components are hidden. Disable - the filter and try again. -
- ); - } else { - return
The selected element is not within an error boundary.
; - } -} diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js index cbd3c5b866746..d23c2953d5593 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js @@ -17,7 +17,6 @@ import {ModalDialogContext} from '../ModalDialog'; import ViewElementSourceContext from './ViewElementSourceContext'; import Toggle from '../Toggle'; import {ElementTypeSuspense} from 'react-devtools-shared/src/types'; -import CannotThrowWarningMessage from './CannotThrowWarningMessage'; import CannotSuspendWarningMessage from './CannotSuspendWarningMessage'; import InspectedElementView from './InspectedElementView'; import {InspectedElementContext} from './InspectedElementContext'; @@ -94,6 +93,9 @@ export default function InspectedElementWrapper(_: Props) { canViewElementSourceFunction(inspectedElement)); const isErrored = inspectedElement != null && inspectedElement.isErrored; + const errorBoundaryID = inspectedElement != null + ? inspectedElement.errorBoundaryID + : null; const isSuspended = element !== null && @@ -108,49 +110,28 @@ export default function InspectedElementWrapper(_: Props) { inspectedElement != null && inspectedElement.canToggleSuspense; 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 (inspectedElement == null || errorBoundaryID == null) { + return; } - // 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({ - id: 'InspectedElement', - type: 'SHOW', - content: , + if (errorBoundaryID !== inspectedElement.id) { + dispatch({ + type: 'SELECT_ELEMENT_BY_ID', + payload: errorBoundaryID, }); - } else { - const nearestErrorBoundaryID = nearestErrorBoundary.id; - - if (nearestErrorBoundary !== element) { - dispatch({ - type: 'SELECT_ELEMENT_BY_ID', - payload: nearestErrorBoundaryID, - }); - } + } - const rendererID = store.getRendererIDForElement(nearestErrorBoundaryID); + const rendererID = store.getRendererIDForElement(errorBoundaryID); - // Toggle error. - if (rendererID !== null) { - bridge.send('overrideError', { - id: nearestErrorBoundaryID, - rendererID, - forceError: !isErrored, - }); - } + // Toggle error. + if (rendererID !== null) { + bridge.send('overrideError', { + id: errorBoundaryID, + rendererID, + forceError: !isErrored, + }); } - }, [bridge, dispatch, element, isErrored, modalDialogDispatch, store]); + }, [bridge, dispatch, isErrored, errorBoundaryID]); // TODO (suspense toggle) Would be nice to eventually use a two setState pattern here as well. const toggleSuspended = useCallback(() => { diff --git a/packages/react-devtools-shared/src/devtools/views/Components/types.js b/packages/react-devtools-shared/src/devtools/views/Components/types.js index 0482cbaa296d8..c97e0bf4bb07e 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/types.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/types.js @@ -31,9 +31,6 @@ export type Element = {| // Should the elements children be visible in the tree? isCollapsed: boolean, - // Is the element an errory boundary? - isErrorBoundary: boolean, - // Owner (if available) ownerID: number, @@ -82,6 +79,7 @@ export type InspectedElement = {| // Is this Error, and can its value be overridden now? isErrored: boolean, canToggleError: boolean, + errorBoundaryID: ?number, // Is this Suspense, and can its value be overridden now? canToggleSuspense: boolean, diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js b/packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js index b5e16c7afd915..1d16aba7fc292 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js @@ -225,8 +225,6 @@ function updateTree( const key = stringTable[keyStringID]; i++; - i++; // isErrorBoundary - if (__DEBUG__) { debug( 'Add', diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index 81d56efd72635..aa60acf06e1f5 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -189,8 +189,6 @@ export function printOperationsArray(operations: Array) { i++; // key - i++; // isErrorBoundary - logs.push( `Add node ${id} (${displayName || 'null'}) as child of ${parentID}`, ); From e3edb365f44b65d4387c6d720a52f46efc4e7d90 Mon Sep 17 00:00:00 2001 From: Bao Pham Date: Wed, 2 Jun 2021 23:17:36 +0800 Subject: [PATCH 4/7] Run prettier --- packages/react-devtools-shared/src/backend/renderer.js | 4 +--- .../src/devtools/views/Components/InspectedElement.js | 5 ++--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index 90a5b36e2540e..3d587c3dd809c 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -3128,9 +3128,7 @@ export function attach( // either that we want to use it to toggle off error state // or that we allow to force error state on it if it's within another // error boundary - errorBoundaryID = isErrored - ? id - : nearestErrorBoundaryID; + errorBoundaryID = isErrored ? id : nearestErrorBoundaryID; } else { errorBoundaryID = nearestErrorBoundaryID; } diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js index d23c2953d5593..fe98d506393b0 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js @@ -93,9 +93,8 @@ export default function InspectedElementWrapper(_: Props) { canViewElementSourceFunction(inspectedElement)); const isErrored = inspectedElement != null && inspectedElement.isErrored; - const errorBoundaryID = inspectedElement != null - ? inspectedElement.errorBoundaryID - : null; + const errorBoundaryID = + inspectedElement != null ? inspectedElement.errorBoundaryID : null; const isSuspended = element !== null && From b78d210854c5826fe60fe104e0196cf356cfeeb8 Mon Sep 17 00:00:00 2001 From: Bao Pham Date: Thu, 3 Jun 2021 15:00:23 +0800 Subject: [PATCH 5/7] Rename to targetErrorBoundaryID and use getFiberIDUnsafe --- .../src/__tests__/inspectedElement-test.js | 22 +++++++++++++------ .../src/backend/legacy/renderer.js | 2 +- .../src/backend/renderer.js | 18 +++++++++------ .../src/backend/types.js | 2 +- .../react-devtools-shared/src/backendAPI.js | 4 ++-- .../views/Components/InspectedElement.js | 16 +++++++------- .../src/devtools/views/Components/types.js | 2 +- 7 files changed, 39 insertions(+), 27 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js index 2d8dc081722af..61ea4414c68b4 100644 --- a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js +++ b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js @@ -2404,7 +2404,9 @@ describe('InspectedElement', () => { ), ); - const errorBoundaryID = ((store.getElementIDAtIndex(0): any): number); + const targetErrorBoundaryID = ((store.getElementIDAtIndex( + 0, + ): any): number); const inspect = index => { // HACK: Recreate TestRenderer instance so we can inspect different // elements @@ -2417,8 +2419,8 @@ describe('InspectedElement', () => { await withErrorsOrWarningsIgnored(['ErrorBoundary'], async () => { await utils.actAsync(() => { bridge.send('overrideError', { - id: errorBoundaryID, - rendererID: store.getRendererIDForElement(errorBoundaryID), + id: targetErrorBoundaryID, + rendererID: store.getRendererIDForElement(targetErrorBoundaryID), forceError, }); }); @@ -2433,13 +2435,15 @@ describe('InspectedElement', () => { // on error boundary itself let inspectedElement = await inspect(0); expect(inspectedElement.canToggleError).toBe(false); - expect(inspectedElement.errorBoundaryID).toBe(null); + expect(inspectedElement.targetErrorBoundaryID).toBe(null); // Inspect inspectedElement = await inspect(1); expect(inspectedElement.canToggleError).toBe(true); expect(inspectedElement.isErrored).toBe(false); - expect(inspectedElement.errorBoundaryID).toBe(errorBoundaryID); + expect(inspectedElement.targetErrorBoundaryID).toBe( + targetErrorBoundaryID, + ); // now force error state on await toggleError(true); @@ -2452,7 +2456,9 @@ describe('InspectedElement', () => { expect(inspectedElement.canToggleError).toBe(true); expect(inspectedElement.isErrored).toBe(true); // its error boundary ID is itself because it's caught the error - expect(inspectedElement.errorBoundaryID).toBe(errorBoundaryID); + expect(inspectedElement.targetErrorBoundaryID).toBe( + targetErrorBoundaryID, + ); await toggleError(false); @@ -2460,7 +2466,9 @@ describe('InspectedElement', () => { inspectedElement = await inspect(1); expect(inspectedElement.canToggleError).toBe(true); expect(inspectedElement.isErrored).toBe(false); - expect(inspectedElement.errorBoundaryID).toBe(errorBoundaryID); + expect(inspectedElement.targetErrorBoundaryID).toBe( + targetErrorBoundaryID, + ); }); }); }); diff --git a/packages/react-devtools-shared/src/backend/legacy/renderer.js b/packages/react-devtools-shared/src/backend/legacy/renderer.js index 36e6459b128c0..7b612ff346171 100644 --- a/packages/react-devtools-shared/src/backend/legacy/renderer.js +++ b/packages/react-devtools-shared/src/backend/legacy/renderer.js @@ -803,7 +803,7 @@ export function attach( // Toggle error boundary did not exist in legacy versions canToggleError: false, isErrored: false, - errorBoundaryID: null, + targetErrorBoundaryID: null, // Suspense did not exist in legacy versions canToggleSuspense: false, diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index 3d587c3dd809c..e23c9e2faf6fc 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -3120,17 +3120,16 @@ export function attach( const isErrored = (fiber.flags & DidCapture) !== NoFlags || forceErrorForFiberIDs.get(id) === true; - const nearestErrorBoundaryID = getNearestErrorBoundaryID(fiber); - let errorBoundaryID; + let targetErrorBoundaryID; if (isErrorBoundary(fiber)) { // if the current inspected element is an error boundary, // either that we want to use it to toggle off error state // or that we allow to force error state on it if it's within another // error boundary - errorBoundaryID = isErrored ? id : nearestErrorBoundaryID; + targetErrorBoundaryID = isErrored ? id : getNearestErrorBoundaryID(fiber); } else { - errorBoundaryID = nearestErrorBoundaryID; + targetErrorBoundaryID = getNearestErrorBoundaryID(fiber); } return { @@ -3150,10 +3149,10 @@ export function attach( canEditFunctionPropsRenamePaths: typeof overridePropsRenamePath === 'function', - canToggleError: supportsTogglingError && errorBoundaryID != null, + canToggleError: supportsTogglingError && targetErrorBoundaryID != null, // Is this error boundary in error state. isErrored, - errorBoundaryID, + targetErrorBoundaryID, canToggleSuspense: supportsTogglingSuspense && @@ -3827,6 +3826,8 @@ export function attach( return null; } + // Map of id and its force error status: true (error), false (toggled off), + // null (do nothing) const forceErrorForFiberIDs = new Map(); function shouldErrorFiberAccordingToMap(fiber) { if (typeof setErrorHandler !== 'function') { @@ -3835,7 +3836,10 @@ export function attach( ); } - const id = getOrGenerateFiberID(fiber); + const id = getFiberIDUnsafe(fiber); + if (id === null) { + return null; + } let status = null; if (forceErrorForFiberIDs.has(id)) { diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index 658f57f959804..d8e0939f1150d 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -229,7 +229,7 @@ export type InspectedElement = {| // Is this Error, and can its value be overridden now? canToggleError: boolean, isErrored: boolean, - errorBoundaryID: ?number, + targetErrorBoundaryID: ?number, // Is this Suspense, and can its value be overridden now? canToggleSuspense: boolean, diff --git a/packages/react-devtools-shared/src/backendAPI.js b/packages/react-devtools-shared/src/backendAPI.js index d1e360ec5485d..4a79147ec23ef 100644 --- a/packages/react-devtools-shared/src/backendAPI.js +++ b/packages/react-devtools-shared/src/backendAPI.js @@ -192,7 +192,7 @@ export function convertInspectedElementBackendToFrontend( canEditHooksAndRenamePaths, canToggleError, isErrored, - errorBoundaryID, + targetErrorBoundaryID, canToggleSuspense, canViewSource, hasLegacyContext, @@ -221,7 +221,7 @@ export function convertInspectedElementBackendToFrontend( canEditHooksAndRenamePaths, canToggleError, isErrored, - errorBoundaryID, + targetErrorBoundaryID, canToggleSuspense, canViewSource, hasLegacyContext, diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js index fe98d506393b0..5cd8be84de3db 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js @@ -93,8 +93,8 @@ export default function InspectedElementWrapper(_: Props) { canViewElementSourceFunction(inspectedElement)); const isErrored = inspectedElement != null && inspectedElement.isErrored; - const errorBoundaryID = - inspectedElement != null ? inspectedElement.errorBoundaryID : null; + const targetErrorBoundaryID = + inspectedElement != null ? inspectedElement.targetErrorBoundaryID : null; const isSuspended = element !== null && @@ -109,28 +109,28 @@ export default function InspectedElementWrapper(_: Props) { inspectedElement != null && inspectedElement.canToggleSuspense; const toggleErrored = useCallback(() => { - if (inspectedElement == null || errorBoundaryID == null) { + if (inspectedElement == null || targetErrorBoundaryID == null) { return; } - if (errorBoundaryID !== inspectedElement.id) { + if (targetErrorBoundaryID !== inspectedElement.id) { dispatch({ type: 'SELECT_ELEMENT_BY_ID', - payload: errorBoundaryID, + payload: targetErrorBoundaryID, }); } - const rendererID = store.getRendererIDForElement(errorBoundaryID); + const rendererID = store.getRendererIDForElement(targetErrorBoundaryID); // Toggle error. if (rendererID !== null) { bridge.send('overrideError', { - id: errorBoundaryID, + id: targetErrorBoundaryID, rendererID, forceError: !isErrored, }); } - }, [bridge, dispatch, isErrored, errorBoundaryID]); + }, [bridge, dispatch, isErrored, targetErrorBoundaryID]); // TODO (suspense toggle) Would be nice to eventually use a two setState pattern here as well. const toggleSuspended = useCallback(() => { diff --git a/packages/react-devtools-shared/src/devtools/views/Components/types.js b/packages/react-devtools-shared/src/devtools/views/Components/types.js index c97e0bf4bb07e..d48e8e32e1f2b 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/types.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/types.js @@ -79,7 +79,7 @@ export type InspectedElement = {| // Is this Error, and can its value be overridden now? isErrored: boolean, canToggleError: boolean, - errorBoundaryID: ?number, + targetErrorBoundaryID: ?number, // Is this Suspense, and can its value be overridden now? canToggleSuspense: boolean, From 094e8747b756c6d60d7b826a83324fe0788df222 Mon Sep 17 00:00:00 2001 From: Bao Pham Date: Thu, 3 Jun 2021 22:48:41 +0800 Subject: [PATCH 6/7] Add TODO for resetting error boundary hack and move the rendererID check --- .../views/Components/InspectedElement.js | 19 ++++++++++--------- .../src/ReactFiberBeginWork.new.js | 2 ++ .../src/ReactFiberBeginWork.old.js | 2 ++ 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js index 5cd8be84de3db..ea976767c02f1 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js @@ -113,17 +113,18 @@ export default function InspectedElementWrapper(_: Props) { return; } - if (targetErrorBoundaryID !== inspectedElement.id) { - dispatch({ - type: 'SELECT_ELEMENT_BY_ID', - payload: targetErrorBoundaryID, - }); - } - const rendererID = store.getRendererIDForElement(targetErrorBoundaryID); - - // Toggle error. if (rendererID !== null) { + if (targetErrorBoundaryID !== inspectedElement.id) { + // Update tree selection so that if we cause a component to error, + // the nearest error boundary will become the newly selected thing. + dispatch({ + type: 'SELECT_ELEMENT_BY_ID', + payload: targetErrorBoundaryID, + }); + } + + // Toggle error. bridge.send('overrideError', { id: targetErrorBoundaryID, rendererID, diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js index bf48a1eb47a1d..30fdc51656363 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.new.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.new.js @@ -957,6 +957,8 @@ function updateClassComponent( case false: { const instance = workInProgress.stateNode; const ctor = workInProgress.type; + // TODO This way of resetting the error boundary state is a hack. + // Is there a better way to do this? const tempInstance = new ctor( workInProgress.memoizedProps, instance.context, diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.old.js b/packages/react-reconciler/src/ReactFiberBeginWork.old.js index cfb7e14beefb7..090fa0b195715 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.old.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.old.js @@ -957,6 +957,8 @@ function updateClassComponent( case false: { const instance = workInProgress.stateNode; const ctor = workInProgress.type; + // TODO This way of resetting the error boundary state is a hack. + // Is there a better way to do this? const tempInstance = new ctor( workInProgress.memoizedProps, instance.context, From 40d62e9e0c1d992310597f4f6b672e5b6adb1a7a Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 3 Jun 2021 11:12:44 -0400 Subject: [PATCH 7/7] Added a comment to kick off CI --- .../src/backend/renderer.js | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index e23c9e2faf6fc..2911855f57bd9 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -3845,12 +3845,21 @@ export function attach( if (forceErrorForFiberIDs.has(id)) { status = forceErrorForFiberIDs.get(id); if (status === false) { + // TRICKY overrideError adds entries to this Map, + // so ideally it would be the method that clears them too, + // but that would break the functionality of the feature, + // since DevTools needs to tell React to act differently than it normally would + // (don't just re-render the failed boundary, but reset its errored state too). + // So we can only clear it after telling React to reset the state. + // Technically this is premature and we should schedule it for later, + // since the render could always fail without committing the updated error boundary, + // but since this is a DEV-only feature, the simplicity is worth the trade off. forceErrorForFiberIDs.delete(id); - } - if (forceErrorForFiberIDs.size === 0) { - // Last override is gone. Switch React back to fast path. - setErrorHandler(shouldErrorFiberAlwaysNull); + if (forceErrorForFiberIDs.size === 0) { + // Last override is gone. Switch React back to fast path. + setErrorHandler(shouldErrorFiberAlwaysNull); + } } } return status;