Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Compute targetErrorBoundaryID lazily when we actually try to set an e…
…rror

This lets us find the nearest error boundary even if it is filtered and
don't have an ID.
  • Loading branch information
sebmarkbage committed Sep 5, 2024
commit da1ad12561f98fc4be82ddf797ef99f99c790eda
Original file line number Diff line number Diff line change
Expand Up @@ -2975,16 +2975,12 @@ describe('InspectedElement', () => {
// Inspect <ErrorBoundary /> and see that we cannot toggle error state
// on error boundary itself
let inspectedElement = await inspect(0);
expect(inspectedElement.canToggleError).toBe(false);
expect(inspectedElement.targetErrorBoundaryID).toBe(null);
expect(inspectedElement.canToggleError).toBe(true);

// Inspect <Example />
inspectedElement = await inspect(1);
expect(inspectedElement.canToggleError).toBe(true);
expect(inspectedElement.isErrored).toBe(false);
expect(inspectedElement.targetErrorBoundaryID).toBe(
targetErrorBoundaryID,
);

// Suppress expected error and warning.
const consoleErrorMock = jest
Expand All @@ -3009,20 +3005,13 @@ describe('InspectedElement', () => {
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.targetErrorBoundaryID).toBe(
targetErrorBoundaryID,
);

await toggleError(false);

// We can now inspect <Example /> with ability to toggle again
inspectedElement = await inspect(1);
expect(inspectedElement.canToggleError).toBe(true);
expect(inspectedElement.isErrored).toBe(false);
expect(inspectedElement.targetErrorBoundaryID).toBe(
targetErrorBoundaryID,
);
});
});

Expand Down
38 changes: 11 additions & 27 deletions packages/react-devtools-shared/src/backend/fiber/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -4202,18 +4202,6 @@ export function attach(
}
}

function getNearestErrorBoundaryID(fiber: Fiber): number | null {
let parent = fiber.return;
while (parent !== null) {
if (isErrorBoundary(parent)) {
// TODO: If this boundary is filtered it won't have an ID.
return getFiberIDUnsafe(parent);
}
parent = parent.return;
}
return null;
}

function inspectElementRaw(id: number): InspectedElement | null {
const devtoolsInstance = idToDevToolsInstanceMap.get(id);
if (devtoolsInstance === undefined) {
Expand Down Expand Up @@ -4409,7 +4397,6 @@ export function attach(
}

let isErrored = false;
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
Expand All @@ -4423,13 +4410,8 @@ export function attach(
isErrored =
(fiber.flags & DidCapture) !== 0 ||
forceErrorForFibers.get(fiber) === true ||
(fiber.alternate !== null &&
forceErrorForFibers.get(fiber.alternate) === true);
targetErrorBoundaryID = isErrored
? fiberInstance.id
: getNearestErrorBoundaryID(fiber);
} else {
targetErrorBoundaryID = getNearestErrorBoundaryID(fiber);
(fiber.alternate !== null &&
forceErrorForFibers.get(fiber.alternate) === true);
}

const plugins: Plugins = {
Expand Down Expand Up @@ -4464,10 +4446,9 @@ export function attach(
canEditFunctionPropsRenamePaths:
typeof overridePropsRenamePath === 'function',

canToggleError: supportsTogglingError && targetErrorBoundaryID != null,
canToggleError: supportsTogglingError,
// Is this error boundary in error state.
isErrored,
targetErrorBoundaryID,

canToggleSuspense:
supportsTogglingSuspense &&
Expand Down Expand Up @@ -4531,11 +4512,9 @@ export function attach(
getOwnersListFromInstance(virtualInstance);

let rootType = null;
let targetErrorBoundaryID = null;
let parent = virtualInstance.parent;
while (parent !== null) {
if (parent.kind !== VIRTUAL_INSTANCE) {
targetErrorBoundaryID = getNearestErrorBoundaryID(parent.data);
let current = parent.data;
while (current.return !== null) {
current = current.return;
Expand Down Expand Up @@ -4564,9 +4543,8 @@ export function attach(
canEditFunctionPropsDeletePaths: false,
canEditFunctionPropsRenamePaths: false,

canToggleError: supportsTogglingError && targetErrorBoundaryID != null,
canToggleError: supportsTogglingError,
isErrored: false,
targetErrorBoundaryID,

canToggleSuspense: supportsTogglingSuspense,

Expand Down Expand Up @@ -5469,7 +5447,13 @@ export function attach(
return;
}
if (devtoolsInstance.kind === FIBER_INSTANCE) {
const fiber = devtoolsInstance.data;
let fiber = devtoolsInstance.data;
while (!isErrorBoundary(fiber)) {
if (fiber.return === null) {
return;
}
fiber = fiber.return;
}
forceErrorForFibers.set(fiber, forceError);
if (fiber.alternate !== null) {
// We only need one of the Fibers in the set.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -826,7 +826,6 @@ export function attach(
// Toggle error boundary did not exist in legacy versions
canToggleError: false,
isErrored: false,
targetErrorBoundaryID: null,

// Suspense did not exist in legacy versions
canToggleSuspense: false,
Expand Down
1 change: 0 additions & 1 deletion packages/react-devtools-shared/src/backend/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,6 @@ export type InspectedElement = {
// Is this Error, and can its value be overridden now?
canToggleError: boolean,
isErrored: boolean,
targetErrorBoundaryID: ?number,

// Is this Suspense, and can its value be overridden now?
canToggleSuspense: boolean,
Expand Down
2 changes: 0 additions & 2 deletions packages/react-devtools-shared/src/backendAPI.js
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,6 @@ export function convertInspectedElementBackendToFrontend(
canEditHooksAndRenamePaths,
canToggleError,
isErrored,
targetErrorBoundaryID,
canToggleSuspense,
canViewSource,
hasLegacyContext,
Expand Down Expand Up @@ -251,7 +250,6 @@ export function convertInspectedElementBackendToFrontend(
canEditHooksAndRenamePaths,
canToggleError,
isErrored,
targetErrorBoundaryID,
canToggleSuspense,
canViewSource,
hasLegacyContext,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,6 @@ export default function InspectedElementWrapper(_: Props): React.Node {
}, [bridge, inspectedElementID, store]);

const isErrored = inspectedElement != null && inspectedElement.isErrored;
const targetErrorBoundaryID =
inspectedElement != null ? inspectedElement.targetErrorBoundaryID : null;

const isSuspended =
element !== null &&
Expand Down Expand Up @@ -137,29 +135,23 @@ export default function InspectedElementWrapper(_: Props): React.Node {
);

const toggleErrored = useCallback(() => {
if (inspectedElement == null || targetErrorBoundaryID == null) {
if (inspectedElement == null) {
return;
}

const rendererID = store.getRendererIDForElement(targetErrorBoundaryID);
const rendererID = store.getRendererIDForElement(inspectedElement.id);
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.
// Because triggering an error will always delete the children, we'll
// automatically select the nearest still mounted instance which will be
// the error boundary.
bridge.send('overrideError', {
id: targetErrorBoundaryID,
id: inspectedElement.id,
rendererID,
forceError: !isErrored,
});
}
}, [bridge, dispatch, isErrored, targetErrorBoundaryID]);
}, [bridge, dispatch, isErrored, inspectedElement]);

// TODO (suspense toggle) Would be nice to eventually use a two setState pattern here as well.
const toggleSuspended = useCallback(() => {
Expand Down
1 change: 0 additions & 1 deletion packages/react-devtools-shared/src/frontend/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,6 @@ export type InspectedElement = {
// Is this Error, and can its value be overridden now?
isErrored: boolean,
canToggleError: boolean,
targetErrorBoundaryID: ?number,

// Is this Suspense, and can its value be overridden now?
canToggleSuspense: boolean,
Expand Down