Skip to content

Commit

Permalink
fix inspecting an element in a nested renderer bug (facebook#24116)
Browse files Browse the repository at this point in the history
Fixes this issue, where inspecting components in nested renderers results in an error. The reason for this is because we have different fiberToIDMap instances for each renderer, and owners of a component could be in different renderers.

This fix moves the fiberToIDMap and idToArbitraryFiberMap out of the attach method so there's only one instance of each for all renderers.
  • Loading branch information
lunaruan authored Mar 17, 2022
1 parent 1c44437 commit 645ec5d
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2734,6 +2734,62 @@ describe('InspectedElement', () => {
});
});

it('inspecting nested renderers should not throw', async () => {
// Ignoring react art warnings
spyOn(console, 'error');
const ReactArt = require('react-art');
const ArtSVGMode = require('art/modes/svg');
const ARTCurrentMode = require('art/modes/current');
store.componentFilters = [];

ARTCurrentMode.setCurrent(ArtSVGMode);
const {Surface, Group} = ReactArt;

function Child() {
return (
<Surface width={1} height={1}>
<Group />
</Surface>
);
}
function App() {
return <Child />;
}

await utils.actAsync(() => {
legacyRender(<App />, document.createElement('div'));
});
expect(store).toMatchInlineSnapshot(`
[root]
▾ <App>
▾ <Child>
▾ <Surface>
<svg>
[root]
<Group>
`);

const inspectedElement = await inspectElementAtIndex(4);
expect(inspectedElement.owners).toMatchInlineSnapshot(`
Array [
Object {
"displayName": "Child",
"hocDisplayNames": null,
"id": 3,
"key": null,
"type": 5,
},
Object {
"displayName": "App",
"hocDisplayNames": null,
"id": 2,
"key": null,
"type": 5,
},
]
`);
});

describe('error boundary', () => {
it('can toggle error', async () => {
class LocalErrorBoundary extends React.Component<any> {
Expand Down
22 changes: 11 additions & 11 deletions packages/react-devtools-shared/src/backend/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -548,6 +548,17 @@ export function getInternalReactConstants(
};
}

// Map of one or more Fibers in a pair to their unique id number.
// We track both Fibers to support Fast Refresh,
// which may forcefully replace one of the pair as part of hot reloading.
// In that case it's still important to be able to locate the previous ID during subsequent renders.
const fiberToIDMap: Map<Fiber, number> = new Map();

// Map of id to one (arbitrary) Fiber in a pair.
// This Map is used to e.g. get the display name for a Fiber or schedule an update,
// operations that should be the same whether the current and work-in-progress Fiber is used.
const idToArbitraryFiberMap: Map<number, Fiber> = new Map();

export function attach(
hook: DevToolsHook,
rendererID: number,
Expand Down Expand Up @@ -1085,17 +1096,6 @@ export function attach(
}
}

// Map of one or more Fibers in a pair to their unique id number.
// We track both Fibers to support Fast Refresh,
// which may forcefully replace one of the pair as part of hot reloading.
// In that case it's still important to be able to locate the previous ID during subsequent renders.
const fiberToIDMap: Map<Fiber, number> = new Map();

// Map of id to one (arbitrary) Fiber in a pair.
// This Map is used to e.g. get the display name for a Fiber or schedule an update,
// operations that should be the same whether the current and work-in-progress Fiber is used.
const idToArbitraryFiberMap: Map<number, Fiber> = new Map();

// When profiling is supported, we store the latest tree base durations for each Fiber.
// This is so that we can quickly capture a snapshot of those values if profiling starts.
// If we didn't store these values, we'd have to crawl the tree when profiling started,
Expand Down

0 comments on commit 645ec5d

Please sign in to comment.