Skip to content

Commit 4717724

Browse files
author
Brian Vaughn
authored
DevTools: Fixed potential cache miss when insepcting elements (#22472)
1 parent 7d38e4f commit 4717724

File tree

8 files changed

+89
-8
lines changed

8 files changed

+89
-8
lines changed

packages/react-devtools-shared/src/__tests__/inspectedElement-test.js

+61
Original file line numberDiff line numberDiff line change
@@ -384,6 +384,67 @@ describe('InspectedElement', () => {
384384
`);
385385
});
386386

387+
// See github.com/facebook/react/issues/22241#issuecomment-931299972
388+
it('should properly recover from a cache miss on the frontend', async () => {
389+
let targetRenderCount = 0;
390+
391+
const Wrapper = ({children}) => children;
392+
const Target = React.memo(props => {
393+
targetRenderCount++;
394+
// Even though his hook isn't referenced, it's used to observe backend rendering.
395+
React.useState(0);
396+
return null;
397+
});
398+
399+
const container = document.createElement('div');
400+
await utils.actAsync(() =>
401+
legacyRender(
402+
<Wrapper>
403+
<Target a={1} b="abc" />
404+
</Wrapper>,
405+
container,
406+
),
407+
);
408+
409+
targetRenderCount = 0;
410+
411+
let inspectedElement = await inspectElementAtIndex(1);
412+
expect(targetRenderCount).toBe(1);
413+
expect(inspectedElement.props).toMatchInlineSnapshot(`
414+
Object {
415+
"a": 1,
416+
"b": "abc",
417+
}
418+
`);
419+
420+
const prevInspectedElement = inspectedElement;
421+
422+
// This test causes an intermediate error to be logged but we can ignore it.
423+
console.error = () => {};
424+
425+
// Wait for our check-for-updates poll to get the new data.
426+
jest.runOnlyPendingTimers();
427+
await Promise.resolve();
428+
429+
// Clear the frontend cache to simulate DevTools being closed and re-opened.
430+
// The backend still thinks the most recently-inspected element is still cached,
431+
// so the frontend needs to tell it to resend a full value.
432+
// We can verify this by asserting that the component is re-rendered again.
433+
testRendererInstance = TestRenderer.create(null, {
434+
unstable_isConcurrent: true,
435+
});
436+
437+
const {
438+
clearCacheForTests,
439+
} = require('react-devtools-shared/src/inspectedElementMutableSource');
440+
clearCacheForTests();
441+
442+
targetRenderCount = 0;
443+
inspectedElement = await inspectElementAtIndex(1);
444+
expect(targetRenderCount).toBe(1);
445+
expect(inspectedElement).toEqual(prevInspectedElement);
446+
});
447+
387448
it('should temporarily disable console logging when re-running a component to inspect its hooks', async () => {
388449
let targetRenderCount = 0;
389450

packages/react-devtools-shared/src/backend/agent.js

+3-1
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ type CopyElementParams = {|
7272
|};
7373

7474
type InspectElementParams = {|
75+
forceFullData: boolean,
7576
id: number,
7677
path: Array<string | number> | null,
7778
rendererID: number,
@@ -346,6 +347,7 @@ export default class Agent extends EventEmitter<{|
346347
};
347348

348349
inspectElement = ({
350+
forceFullData,
349351
id,
350352
path,
351353
rendererID,
@@ -357,7 +359,7 @@ export default class Agent extends EventEmitter<{|
357359
} else {
358360
this._bridge.send(
359361
'inspectedElement',
360-
renderer.inspectElement(requestID, id, path),
362+
renderer.inspectElement(requestID, id, path, forceFullData),
361363
);
362364

363365
// When user selects an element, stop trying to restore the selection,

packages/react-devtools-shared/src/backend/legacy/renderer.js

+2-1
Original file line numberDiff line numberDiff line change
@@ -694,8 +694,9 @@ export function attach(
694694
requestID: number,
695695
id: number,
696696
path: Array<string | number> | null,
697+
forceFullData: boolean,
697698
): InspectedElementPayload {
698-
if (currentlyInspectedElementID !== id) {
699+
if (forceFullData || currentlyInspectedElementID !== id) {
699700
currentlyInspectedElementID = id;
700701
currentlyInspectedPaths = {};
701702
}

packages/react-devtools-shared/src/backend/renderer.js

+2-1
Original file line numberDiff line numberDiff line change
@@ -3439,12 +3439,13 @@ export function attach(
34393439
requestID: number,
34403440
id: number,
34413441
path: Array<string | number> | null,
3442+
forceFullData: boolean,
34423443
): InspectedElementPayload {
34433444
if (path !== null) {
34443445
mergeInspectedPaths(path);
34453446
}
34463447

3447-
if (isMostRecentlyInspectedElement(id)) {
3448+
if (isMostRecentlyInspectedElement(id) && !forceFullData) {
34483449
if (!hasElementUpdatedSinceLastInspected) {
34493450
if (path !== null) {
34503451
let secondaryCategory = null;

packages/react-devtools-shared/src/backend/types.js

+1
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,7 @@ export type RendererInterface = {
339339
requestID: number,
340340
id: number,
341341
inspectedPaths: Object,
342+
forceFullData: boolean,
342343
) => InspectedElementPayload,
343344
logElementToConsole: (id: number) => void,
344345
overrideError: (id: number, forceError: boolean) => void,

packages/react-devtools-shared/src/backendAPI.js

+3
Original file line numberDiff line numberDiff line change
@@ -86,11 +86,13 @@ export function copyInspectedElementPath({
8686

8787
export function inspectElement({
8888
bridge,
89+
forceFullData,
8990
id,
9091
path,
9192
rendererID,
9293
}: {|
9394
bridge: FrontendBridge,
95+
forceFullData: boolean,
9496
id: number,
9597
path: Array<string | number> | null,
9698
rendererID: number,
@@ -103,6 +105,7 @@ export function inspectElement({
103105
);
104106

105107
bridge.send('inspectElement', {
108+
forceFullData,
106109
id,
107110
path,
108111
rendererID,

packages/react-devtools-shared/src/bridge.js

+1
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,7 @@ type ViewAttributeSourceParams = {|
138138

139139
type InspectElementParams = {|
140140
...ElementAndRendererID,
141+
forceFullData: boolean,
141142
path: Array<number | string> | null,
142143
requestID: number,
143144
|};

packages/react-devtools-shared/src/inspectedElementMutableSource.js

+16-5
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,15 @@ export function inspectElement({
6262
rendererID: number,
6363
|}): Promise<InspectElementReturnType> {
6464
const {id} = element;
65+
66+
// This could indicate that the DevTools UI has been closed and reopened.
67+
// The in-memory cache will be clear but the backend still thinks we have cached data.
68+
// In this case, we need to tell it to resend the full data.
69+
const forceFullData = !inspectedElementCache.has(id);
70+
6571
return inspectElementAPI({
6672
bridge,
73+
forceFullData,
6774
id,
6875
path,
6976
rendererID,
@@ -74,7 +81,7 @@ export function inspectElement({
7481
switch (type) {
7582
case 'no-change':
7683
// This is a no-op for the purposes of our cache.
77-
inspectedElement = inspectedElementCache.get(element.id);
84+
inspectedElement = inspectedElementCache.get(id);
7885
if (inspectedElement != null) {
7986
return [inspectedElement, type];
8087
}
@@ -85,7 +92,7 @@ export function inspectElement({
8592
case 'not-found':
8693
// This is effectively a no-op.
8794
// If the Element is still in the Store, we can eagerly remove it from the Map.
88-
inspectedElementCache.remove(element.id);
95+
inspectedElementCache.remove(id);
8996

9097
throw Error(`Element "${id}" not found`);
9198

@@ -98,7 +105,7 @@ export function inspectElement({
98105
fullData.value,
99106
);
100107

101-
inspectedElementCache.set(element.id, inspectedElement);
108+
inspectedElementCache.set(id, inspectedElement);
102109

103110
return [inspectedElement, type];
104111

@@ -108,7 +115,7 @@ export function inspectElement({
108115

109116
// A path has been hydrated.
110117
// Merge it with the latest copy we have locally and resolve with the merged value.
111-
inspectedElement = inspectedElementCache.get(element.id) || null;
118+
inspectedElement = inspectedElementCache.get(id) || null;
112119
if (inspectedElement !== null) {
113120
// Clone element
114121
inspectedElement = {...inspectedElement};
@@ -121,7 +128,7 @@ export function inspectElement({
121128
hydrateHelper(value, ((path: any): Path)),
122129
);
123130

124-
inspectedElementCache.set(element.id, inspectedElement);
131+
inspectedElementCache.set(id, inspectedElement);
125132

126133
return [inspectedElement, type];
127134
}
@@ -140,3 +147,7 @@ export function inspectElement({
140147
throw Error(`Unable to inspect element with id "${id}"`);
141148
});
142149
}
150+
151+
export function clearCacheForTests(): void {
152+
inspectedElementCache.reset();
153+
}

0 commit comments

Comments
 (0)