From 0931c95a5d0e15fa0b309b13552cb03cd5ee91ab Mon Sep 17 00:00:00 2001 From: Nick Reiley Date: Thu, 14 May 2020 00:02:33 +0500 Subject: [PATCH 1/4] skip reading element for imported data --- .../src/__tests__/profilingUtils-test.js | 2 +- .../react-devtools-shared/src/__tests__/utils.js | 12 +++++++----- packages/react-devtools-shared/src/devtools/store.js | 5 +++++ .../views/Profiler/ProfilingImportExportButtons.js | 8 ++++---- .../src/devtools/views/Profiler/types.js | 1 + .../src/devtools/views/Profiler/utils.js | 12 ++++++------ 6 files changed, 24 insertions(+), 16 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/profilingUtils-test.js b/packages/react-devtools-shared/src/__tests__/profilingUtils-test.js index 5cf07cf472e..08b840dfe34 100644 --- a/packages/react-devtools-shared/src/__tests__/profilingUtils-test.js +++ b/packages/react-devtools-shared/src/__tests__/profilingUtils-test.js @@ -16,7 +16,7 @@ describe('profiling utils', () => { it('should throw if importing older/unsupported data', () => { expect(() => - utils.prepareProfilingDataFrontendFromExport( + utils.prepareProfilingDataFrontendFromImport( ({ version: 0, dataForRoots: [], diff --git a/packages/react-devtools-shared/src/__tests__/utils.js b/packages/react-devtools-shared/src/__tests__/utils.js index 0b3f03a7fb3..78c73b06a69 100644 --- a/packages/react-devtools-shared/src/__tests__/utils.js +++ b/packages/react-devtools-shared/src/__tests__/utils.js @@ -173,7 +173,7 @@ export function requireTestRenderer(): ReactTestRenderer { export function exportImportHelper(bridge: FrontendBridge, store: Store): void { const { prepareProfilingDataExport, - prepareProfilingDataFrontendFromExport, + prepareProfilingDataFrontendFromImport, } = require('react-devtools-shared/src/devtools/views/Profiler/utils'); const {profilerStore} = store; @@ -194,18 +194,20 @@ export function exportImportHelper(bridge: FrontendBridge, store: Store): void { ); const parsedProfilingDataExport = JSON.parse(serializedProfilingDataExport); - const profilingDataFrontend = prepareProfilingDataFrontendFromExport( + const profilingDataFrontendImport = prepareProfilingDataFrontendFromImport( (parsedProfilingDataExport: any), ); // Sanity check that profiling snapshots are serialized correctly. - expect(profilingDataFrontendInitial).toEqual(profilingDataFrontend); + expect(profilingDataFrontendInitial.dataForRoots).toEqual( + profilingDataFrontendImport.dataForRoots, + ); // Snapshot the JSON-parsed object, rather than the raw string, because Jest formats the diff nicer. expect(parsedProfilingDataExport).toMatchSnapshot('imported data'); act(() => { - // Apply the new exported-then-reimported data so tests can re-run assertions. - profilerStore.profilingData = profilingDataFrontend; + // Apply the new exported-then-imported data so tests can re-run assertions. + profilerStore.profilingData = profilingDataFrontendImport; }); } diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js index 3d98d0ff154..d05b6feedea 100644 --- a/packages/react-devtools-shared/src/devtools/store.js +++ b/packages/react-devtools-shared/src/devtools/store.js @@ -416,6 +416,11 @@ export default class Store extends EventEmitter<{| } getElementByID(id: number): Element | null { + if ( + this._profilerStore._dataFrontend && + this._profilerStore._dataFrontend.imported === true + ) + return null; const element = this._idToElement.get(id); if (element == null) { console.warn(`No element found with id "${id}"`); diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilingImportExportButtons.js b/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilingImportExportButtons.js index 7f14438aaa6..7b22e7561d9 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilingImportExportButtons.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilingImportExportButtons.js @@ -16,7 +16,7 @@ import ButtonIcon from '../ButtonIcon'; import {StoreContext} from '../context'; import { prepareProfilingDataExport, - prepareProfilingDataFrontendFromExport, + prepareProfilingDataFrontendFromImport, } from './utils'; import {downloadFile} from '../utils'; @@ -77,11 +77,11 @@ export default function ProfilingImportExportButtons() { fileReader.addEventListener('load', () => { try { const raw = ((fileReader.result: any): string); - const profilingDataExport = ((JSON.parse( + const profilingDataImport = ((JSON.parse( raw, ): any): ProfilingDataExport); - profilerStore.profilingData = prepareProfilingDataFrontendFromExport( - profilingDataExport, + profilerStore.profilingData = prepareProfilingDataFrontendFromImport( + profilingDataImport, ); } catch (error) { modalDialogDispatch({ diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/types.js b/packages/react-devtools-shared/src/devtools/views/Profiler/types.js index 4156aa592c4..ab63f093ed2 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/types.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/types.js @@ -105,6 +105,7 @@ export type ProfilingDataForRootFrontend = {| export type ProfilingDataFrontend = {| // Profiling data per root. dataForRoots: Map, + imported: boolean, |}; export type CommitDataExport = {| diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js b/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js index c453fc166cf..acac810ca13 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js @@ -99,21 +99,21 @@ export function prepareProfilingDataFrontendFromBackendAndStore( ); }); - return {dataForRoots}; + return {dataForRoots, imported: false}; } // Converts a Profiling data export into the format required by the Store. -export function prepareProfilingDataFrontendFromExport( - profilingDataExport: ProfilingDataExport, +export function prepareProfilingDataFrontendFromImport( + profilingDataImport: ProfilingDataExport, ): ProfilingDataFrontend { - const {version} = profilingDataExport; + const {version} = profilingDataImport; if (version !== PROFILER_EXPORT_VERSION) { throw Error(`Unsupported profiler export version "${version}"`); } const dataForRoots: Map = new Map(); - profilingDataExport.dataForRoots.forEach( + profilingDataImport.dataForRoots.forEach( ({ commitData, displayName, @@ -156,7 +156,7 @@ export function prepareProfilingDataFrontendFromExport( }, ); - return {dataForRoots}; + return {dataForRoots, imported: true}; } // Converts a Store Profiling data into a format that can be safely (JSON) serialized for export. From deaa490c50e3b6aef4042eeb9893ab61f93b584f Mon Sep 17 00:00:00 2001 From: Nick Reiley Date: Thu, 14 May 2020 21:37:44 +0500 Subject: [PATCH 2/4] rename nodes & enable store lookup for components tab --- .../src/__tests__/profilingUtils-test.js | 2 +- packages/react-devtools-shared/src/__tests__/utils.js | 4 ++-- packages/react-devtools-shared/src/devtools/store.js | 5 ----- .../src/devtools/views/Profiler/ProfilerContext.js | 8 ++++++-- .../views/Profiler/ProfilingImportExportButtons.js | 4 ++-- .../src/devtools/views/Profiler/utils.js | 2 +- 6 files changed, 12 insertions(+), 13 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/profilingUtils-test.js b/packages/react-devtools-shared/src/__tests__/profilingUtils-test.js index 08b840dfe34..1cda2fed377 100644 --- a/packages/react-devtools-shared/src/__tests__/profilingUtils-test.js +++ b/packages/react-devtools-shared/src/__tests__/profilingUtils-test.js @@ -16,7 +16,7 @@ describe('profiling utils', () => { it('should throw if importing older/unsupported data', () => { expect(() => - utils.prepareProfilingDataFrontendFromImport( + utils.prepareProfilingDataFrontendForImport( ({ version: 0, dataForRoots: [], diff --git a/packages/react-devtools-shared/src/__tests__/utils.js b/packages/react-devtools-shared/src/__tests__/utils.js index 78c73b06a69..c685263fdbe 100644 --- a/packages/react-devtools-shared/src/__tests__/utils.js +++ b/packages/react-devtools-shared/src/__tests__/utils.js @@ -173,7 +173,7 @@ export function requireTestRenderer(): ReactTestRenderer { export function exportImportHelper(bridge: FrontendBridge, store: Store): void { const { prepareProfilingDataExport, - prepareProfilingDataFrontendFromImport, + prepareProfilingDataFrontendForImport, } = require('react-devtools-shared/src/devtools/views/Profiler/utils'); const {profilerStore} = store; @@ -194,7 +194,7 @@ export function exportImportHelper(bridge: FrontendBridge, store: Store): void { ); const parsedProfilingDataExport = JSON.parse(serializedProfilingDataExport); - const profilingDataFrontendImport = prepareProfilingDataFrontendFromImport( + const profilingDataFrontendImport = prepareProfilingDataFrontendForImport( (parsedProfilingDataExport: any), ); diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js index d05b6feedea..3d98d0ff154 100644 --- a/packages/react-devtools-shared/src/devtools/store.js +++ b/packages/react-devtools-shared/src/devtools/store.js @@ -416,11 +416,6 @@ export default class Store extends EventEmitter<{| } getElementByID(id: number): Element | null { - if ( - this._profilerStore._dataFrontend && - this._profilerStore._dataFrontend.imported === true - ) - return null; const element = this._idToElement.get(id); if (element == null) { console.warn(`No element found with id "${id}"`); diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilerContext.js b/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilerContext.js index 1108a6bb64f..6c07d065077 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilerContext.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilerContext.js @@ -138,7 +138,11 @@ function ProfilerContextController({children}: Props) { selectFiberName(name); // Sync selection to the Components tab for convenience. - if (id !== null) { + if ( + id !== null && + profilingData !== null && + profilingData.imported === false + ) { const element = store.getElementByID(id); // Keep in mind that profiling data may be from a previous session. @@ -151,7 +155,7 @@ function ProfilerContextController({children}: Props) { } } }, - [dispatch, selectFiberID, selectFiberName, store], + [dispatch, selectFiberID, selectFiberName, store, profilingData], ); const setRootIDAndClearFiber = useCallback( diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilingImportExportButtons.js b/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilingImportExportButtons.js index 7b22e7561d9..ba4ba20fe57 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilingImportExportButtons.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilingImportExportButtons.js @@ -16,7 +16,7 @@ import ButtonIcon from '../ButtonIcon'; import {StoreContext} from '../context'; import { prepareProfilingDataExport, - prepareProfilingDataFrontendFromImport, + prepareProfilingDataFrontendForImport, } from './utils'; import {downloadFile} from '../utils'; @@ -80,7 +80,7 @@ export default function ProfilingImportExportButtons() { const profilingDataImport = ((JSON.parse( raw, ): any): ProfilingDataExport); - profilerStore.profilingData = prepareProfilingDataFrontendFromImport( + profilerStore.profilingData = prepareProfilingDataFrontendForImport( profilingDataImport, ); } catch (error) { diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js b/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js index acac810ca13..62fa3159580 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js @@ -103,7 +103,7 @@ export function prepareProfilingDataFrontendFromBackendAndStore( } // Converts a Profiling data export into the format required by the Store. -export function prepareProfilingDataFrontendFromImport( +export function prepareProfilingDataFrontendForImport( profilingDataImport: ProfilingDataExport, ): ProfilingDataFrontend { const {version} = profilingDataImport; From d83e15af36dc3af18ef61ad172daf3da5bd1bebc Mon Sep 17 00:00:00 2001 From: Nick Reiley Date: Thu, 14 May 2020 23:00:24 +0500 Subject: [PATCH 3/4] replace names --- .../devtools/views/Profiler/ProfilingImportExportButtons.js | 4 ++-- .../src/devtools/views/Profiler/utils.js | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilingImportExportButtons.js b/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilingImportExportButtons.js index ba4ba20fe57..77c3aba4f48 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilingImportExportButtons.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilingImportExportButtons.js @@ -77,11 +77,11 @@ export default function ProfilingImportExportButtons() { fileReader.addEventListener('load', () => { try { const raw = ((fileReader.result: any): string); - const profilingDataImport = ((JSON.parse( + const profilingDataExport = ((JSON.parse( raw, ): any): ProfilingDataExport); profilerStore.profilingData = prepareProfilingDataFrontendForImport( - profilingDataImport, + profilingDataExport, ); } catch (error) { modalDialogDispatch({ diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js b/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js index 62fa3159580..110d5aba536 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js @@ -104,16 +104,16 @@ export function prepareProfilingDataFrontendFromBackendAndStore( // Converts a Profiling data export into the format required by the Store. export function prepareProfilingDataFrontendForImport( - profilingDataImport: ProfilingDataExport, + profilingDataExport: ProfilingDataExport, ): ProfilingDataFrontend { - const {version} = profilingDataImport; + const {version} = profilingDataExport; if (version !== PROFILER_EXPORT_VERSION) { throw Error(`Unsupported profiler export version "${version}"`); } const dataForRoots: Map = new Map(); - profilingDataImport.dataForRoots.forEach( + profilingDataExport.dataForRoots.forEach( ({ commitData, displayName, From 9761ee58143850f5075eb3b658f9cc5a0322becd Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 15 May 2020 09:49:40 -0700 Subject: [PATCH 4/4] Added some more test coverage; reverted rename --- .../src/__tests__/profilerContext-test.js | 8 -------- .../src/__tests__/profilingUtils-test.js | 2 +- packages/react-devtools-shared/src/__tests__/utils.js | 10 ++++++---- .../src/devtools/views/Profiler/ProfilerContext.js | 10 +++++----- .../views/Profiler/ProfilingImportExportButtons.js | 4 ++-- .../src/devtools/views/Profiler/utils.js | 2 +- 6 files changed, 15 insertions(+), 21 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/profilerContext-test.js b/packages/react-devtools-shared/src/__tests__/profilerContext-test.js index 35bc06e5891..a65b65e5b7c 100644 --- a/packages/react-devtools-shared/src/__tests__/profilerContext-test.js +++ b/packages/react-devtools-shared/src/__tests__/profilerContext-test.js @@ -337,18 +337,10 @@ describe('ProfilerContext', () => { await utils.actAsync(() => context.selectFiber(parentID, 'Parent')); expect(selectedElementID).toBe(parentID); - // We expect a "no element found" warning. - // Let's hide it from the test console though. - spyOn(console, 'warn'); - // Select an unmounted element and verify no Components tab selection doesn't change. await utils.actAsync(() => context.selectFiber(childID, 'Child')); expect(selectedElementID).toBe(parentID); - expect(console.warn).toHaveBeenCalledWith( - `No element found with id "${childID}"`, - ); - done(); }); }); diff --git a/packages/react-devtools-shared/src/__tests__/profilingUtils-test.js b/packages/react-devtools-shared/src/__tests__/profilingUtils-test.js index 1cda2fed377..5cf07cf472e 100644 --- a/packages/react-devtools-shared/src/__tests__/profilingUtils-test.js +++ b/packages/react-devtools-shared/src/__tests__/profilingUtils-test.js @@ -16,7 +16,7 @@ describe('profiling utils', () => { it('should throw if importing older/unsupported data', () => { expect(() => - utils.prepareProfilingDataFrontendForImport( + utils.prepareProfilingDataFrontendFromExport( ({ version: 0, dataForRoots: [], diff --git a/packages/react-devtools-shared/src/__tests__/utils.js b/packages/react-devtools-shared/src/__tests__/utils.js index c685263fdbe..550ae111098 100644 --- a/packages/react-devtools-shared/src/__tests__/utils.js +++ b/packages/react-devtools-shared/src/__tests__/utils.js @@ -173,7 +173,7 @@ export function requireTestRenderer(): ReactTestRenderer { export function exportImportHelper(bridge: FrontendBridge, store: Store): void { const { prepareProfilingDataExport, - prepareProfilingDataFrontendForImport, + prepareProfilingDataFrontendFromExport, } = require('react-devtools-shared/src/devtools/views/Profiler/utils'); const {profilerStore} = store; @@ -181,6 +181,7 @@ export function exportImportHelper(bridge: FrontendBridge, store: Store): void { expect(profilerStore.profilingData).not.toBeNull(); const profilingDataFrontendInitial = ((profilerStore.profilingData: any): ProfilingDataFrontend); + expect(profilingDataFrontendInitial.imported).toBe(false); const profilingDataExport = prepareProfilingDataExport( profilingDataFrontendInitial, @@ -194,13 +195,14 @@ export function exportImportHelper(bridge: FrontendBridge, store: Store): void { ); const parsedProfilingDataExport = JSON.parse(serializedProfilingDataExport); - const profilingDataFrontendImport = prepareProfilingDataFrontendForImport( + const profilingDataFrontend = prepareProfilingDataFrontendFromExport( (parsedProfilingDataExport: any), ); + expect(profilingDataFrontend.imported).toBe(true); // Sanity check that profiling snapshots are serialized correctly. expect(profilingDataFrontendInitial.dataForRoots).toEqual( - profilingDataFrontendImport.dataForRoots, + profilingDataFrontend.dataForRoots, ); // Snapshot the JSON-parsed object, rather than the raw string, because Jest formats the diff nicer. @@ -208,6 +210,6 @@ export function exportImportHelper(bridge: FrontendBridge, store: Store): void { act(() => { // Apply the new exported-then-imported data so tests can re-run assertions. - profilerStore.profilingData = profilingDataFrontendImport; + profilerStore.profilingData = profilingDataFrontend; }); } diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilerContext.js b/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilerContext.js index 6c07d065077..ac3b58f1879 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilerContext.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilerContext.js @@ -138,16 +138,16 @@ function ProfilerContextController({children}: Props) { selectFiberName(name); // Sync selection to the Components tab for convenience. + // Keep in mind that profiling data may be from a previous session. + // If data has been imported, we should skip the selection sync. if ( id !== null && profilingData !== null && profilingData.imported === false ) { - const element = store.getElementByID(id); - - // Keep in mind that profiling data may be from a previous session. - // In that case, IDs may match up arbitrarily; to be safe, compare both ID and display name. - if (element !== null && element.displayName === name) { + // We should still check to see if this element is still in the store. + // It may have been removed during profiling. + if (store.containsElement(id)) { dispatch({ type: 'SELECT_ELEMENT_BY_ID', payload: id, diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilingImportExportButtons.js b/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilingImportExportButtons.js index 77c3aba4f48..7f14438aaa6 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilingImportExportButtons.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilingImportExportButtons.js @@ -16,7 +16,7 @@ import ButtonIcon from '../ButtonIcon'; import {StoreContext} from '../context'; import { prepareProfilingDataExport, - prepareProfilingDataFrontendForImport, + prepareProfilingDataFrontendFromExport, } from './utils'; import {downloadFile} from '../utils'; @@ -80,7 +80,7 @@ export default function ProfilingImportExportButtons() { const profilingDataExport = ((JSON.parse( raw, ): any): ProfilingDataExport); - profilerStore.profilingData = prepareProfilingDataFrontendForImport( + profilerStore.profilingData = prepareProfilingDataFrontendFromExport( profilingDataExport, ); } catch (error) { diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js b/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js index 110d5aba536..9691af89a58 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js @@ -103,7 +103,7 @@ export function prepareProfilingDataFrontendFromBackendAndStore( } // Converts a Profiling data export into the format required by the Store. -export function prepareProfilingDataFrontendForImport( +export function prepareProfilingDataFrontendFromExport( profilingDataExport: ProfilingDataExport, ): ProfilingDataFrontend { const {version} = profilingDataExport;