From 4f2096f42bb30e28782c11182e25722e60e5589e Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 30 Sep 2021 12:02:16 -0400 Subject: [PATCH] DevTools: Fixed potential cache miss when insepcting elements If the DevTools UI is closed and reopened, the in-memory insepected element cache will be reset. The backend assumes this data is cached though, and may send a 'no-change' response if DevTools are re-opened and the element is re-inspected. To fix this, the frontend sends an explicit 'force' flag to tell the backend to always resend the full data. --- .../src/__tests__/inspectedElement-test.js | 61 +++++++++++++++++++ .../src/backend/agent.js | 4 +- .../src/backend/legacy/renderer.js | 3 +- .../src/backend/renderer.js | 3 +- .../src/backend/types.js | 1 + .../react-devtools-shared/src/backendAPI.js | 3 + packages/react-devtools-shared/src/bridge.js | 1 + .../src/inspectedElementMutableSource.js | 21 +++++-- 8 files changed, 89 insertions(+), 8 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js index eb196e76b480a..211cd27b1522f 100644 --- a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js +++ b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js @@ -384,6 +384,67 @@ describe('InspectedElement', () => { `); }); + // See github.com/facebook/react/issues/22241#issuecomment-931299972 + it('should properly recover from a cache miss on the frontend', async () => { + let targetRenderCount = 0; + + const Wrapper = ({children}) => children; + const Target = React.memo(props => { + targetRenderCount++; + // Even though his hook isn't referenced, it's used to observe backend rendering. + React.useState(0); + return null; + }); + + const container = document.createElement('div'); + await utils.actAsync(() => + legacyRender( + + + , + container, + ), + ); + + targetRenderCount = 0; + + let inspectedElement = await inspectElementAtIndex(1); + expect(targetRenderCount).toBe(1); + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "a": 1, + "b": "abc", + } + `); + + const prevInspectedElement = inspectedElement; + + // This test causes an intermediate error to be logged but we can ignore it. + console.error = () => {}; + + // Wait for our check-for-updates poll to get the new data. + jest.runOnlyPendingTimers(); + await Promise.resolve(); + + // Clear the frontend cache to simulate DevTools being closed and re-opened. + // The backend still thinks the most recently-inspected element is still cached, + // so the frontend needs to tell it to resend a full value. + // We can verify this by asserting that the component is re-rendered again. + testRendererInstance = TestRenderer.create(null, { + unstable_isConcurrent: true, + }); + + const { + clearCacheForTests, + } = require('react-devtools-shared/src/inspectedElementMutableSource'); + clearCacheForTests(); + + targetRenderCount = 0; + inspectedElement = await inspectElementAtIndex(1); + expect(targetRenderCount).toBe(1); + expect(inspectedElement).toEqual(prevInspectedElement); + }); + it('should temporarily disable console logging when re-running a component to inspect its hooks', async () => { let targetRenderCount = 0; diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js index 9eea501cbdd0a..fdd46bf9c88f0 100644 --- a/packages/react-devtools-shared/src/backend/agent.js +++ b/packages/react-devtools-shared/src/backend/agent.js @@ -72,6 +72,7 @@ type CopyElementParams = {| |}; type InspectElementParams = {| + forceFullData: boolean, id: number, path: Array | null, rendererID: number, @@ -346,6 +347,7 @@ export default class Agent extends EventEmitter<{| }; inspectElement = ({ + forceFullData, id, path, rendererID, @@ -357,7 +359,7 @@ export default class Agent extends EventEmitter<{| } else { this._bridge.send( 'inspectedElement', - renderer.inspectElement(requestID, id, path), + renderer.inspectElement(requestID, id, path, forceFullData), ); // When user selects an element, stop trying to restore the selection, diff --git a/packages/react-devtools-shared/src/backend/legacy/renderer.js b/packages/react-devtools-shared/src/backend/legacy/renderer.js index 1f0aed404087a..d799ce72a275c 100644 --- a/packages/react-devtools-shared/src/backend/legacy/renderer.js +++ b/packages/react-devtools-shared/src/backend/legacy/renderer.js @@ -694,8 +694,9 @@ export function attach( requestID: number, id: number, path: Array | null, + forceFullData: boolean, ): InspectedElementPayload { - if (currentlyInspectedElementID !== id) { + if (forceFullData || currentlyInspectedElementID !== id) { currentlyInspectedElementID = id; currentlyInspectedPaths = {}; } diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index a2469577a0a38..011f055e505a8 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -3439,12 +3439,13 @@ export function attach( requestID: number, id: number, path: Array | null, + forceFullData: boolean, ): InspectedElementPayload { if (path !== null) { mergeInspectedPaths(path); } - if (isMostRecentlyInspectedElement(id)) { + if (isMostRecentlyInspectedElement(id) && !forceFullData) { if (!hasElementUpdatedSinceLastInspected) { if (path !== null) { let secondaryCategory = null; diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index 2206dd94fb124..bbf574ed21a50 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -339,6 +339,7 @@ export type RendererInterface = { requestID: number, id: number, inspectedPaths: Object, + forceFullData: boolean, ) => InspectedElementPayload, logElementToConsole: (id: number) => void, overrideError: (id: number, forceError: boolean) => void, diff --git a/packages/react-devtools-shared/src/backendAPI.js b/packages/react-devtools-shared/src/backendAPI.js index 5e0ac663fee44..2aa5b30cd22e6 100644 --- a/packages/react-devtools-shared/src/backendAPI.js +++ b/packages/react-devtools-shared/src/backendAPI.js @@ -86,11 +86,13 @@ export function copyInspectedElementPath({ export function inspectElement({ bridge, + forceFullData, id, path, rendererID, }: {| bridge: FrontendBridge, + forceFullData: boolean, id: number, path: Array | null, rendererID: number, @@ -103,6 +105,7 @@ export function inspectElement({ ); bridge.send('inspectElement', { + forceFullData, id, path, rendererID, diff --git a/packages/react-devtools-shared/src/bridge.js b/packages/react-devtools-shared/src/bridge.js index ae5abf7c4f2b5..e67f6cee95423 100644 --- a/packages/react-devtools-shared/src/bridge.js +++ b/packages/react-devtools-shared/src/bridge.js @@ -138,6 +138,7 @@ type ViewAttributeSourceParams = {| type InspectElementParams = {| ...ElementAndRendererID, + forceFullData: boolean, path: Array | null, requestID: number, |}; diff --git a/packages/react-devtools-shared/src/inspectedElementMutableSource.js b/packages/react-devtools-shared/src/inspectedElementMutableSource.js index d0c2880005950..9302cd782a0cf 100644 --- a/packages/react-devtools-shared/src/inspectedElementMutableSource.js +++ b/packages/react-devtools-shared/src/inspectedElementMutableSource.js @@ -62,8 +62,15 @@ export function inspectElement({ rendererID: number, |}): Promise { const {id} = element; + + // This could indicate that the DevTools UI has been closed and reopened. + // The in-memory cache will be clear but the backend still thinks we have cached data. + // In this case, we need to tell it to resend the full data. + const forceFullData = !inspectedElementCache.has(id); + return inspectElementAPI({ bridge, + forceFullData, id, path, rendererID, @@ -74,7 +81,7 @@ export function inspectElement({ switch (type) { case 'no-change': // This is a no-op for the purposes of our cache. - inspectedElement = inspectedElementCache.get(element.id); + inspectedElement = inspectedElementCache.get(id); if (inspectedElement != null) { return [inspectedElement, type]; } @@ -85,7 +92,7 @@ export function inspectElement({ case 'not-found': // This is effectively a no-op. // If the Element is still in the Store, we can eagerly remove it from the Map. - inspectedElementCache.remove(element.id); + inspectedElementCache.remove(id); throw Error(`Element "${id}" not found`); @@ -98,7 +105,7 @@ export function inspectElement({ fullData.value, ); - inspectedElementCache.set(element.id, inspectedElement); + inspectedElementCache.set(id, inspectedElement); return [inspectedElement, type]; @@ -108,7 +115,7 @@ export function inspectElement({ // A path has been hydrated. // Merge it with the latest copy we have locally and resolve with the merged value. - inspectedElement = inspectedElementCache.get(element.id) || null; + inspectedElement = inspectedElementCache.get(id) || null; if (inspectedElement !== null) { // Clone element inspectedElement = {...inspectedElement}; @@ -121,7 +128,7 @@ export function inspectElement({ hydrateHelper(value, ((path: any): Path)), ); - inspectedElementCache.set(element.id, inspectedElement); + inspectedElementCache.set(id, inspectedElement); return [inspectedElement, type]; } @@ -140,3 +147,7 @@ export function inspectElement({ throw Error(`Unable to inspect element with id "${id}"`); }); } + +export function clearCacheForTests(): void { + inspectedElementCache.reset(); +}