From 09029acf009aeecdd10bbaeddfe3463568d23c0d Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 25 Apr 2019 12:16:13 -0700 Subject: [PATCH] Refactored owners list to use cached metadata (and added more tests) --- .../__snapshots__/storeOwners-test.js.snap | 105 ++++++++++++++- src/__tests__/storeOwners-test.js | 123 +++++++++++++++++- src/devtools/store.js | 112 +++++++++++----- 3 files changed, 306 insertions(+), 34 deletions(-) diff --git a/src/__tests__/__snapshots__/storeOwners-test.js.snap b/src/__tests__/__snapshots__/storeOwners-test.js.snap index cbb2177c9b..28069711fd 100644 --- a/src/__tests__/__snapshots__/storeOwners-test.js.snap +++ b/src/__tests__/__snapshots__/storeOwners-test.js.snap @@ -1,20 +1,121 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`Store owners list should drill through interleaved intermediate components: 1: mount 1`] = ` +[root] + ▾ + ▾ + + ▾ + + +`; + +exports[`Store owners list should drill through interleaved intermediate components: 2: components owned by 1`] = ` +" ▾ + ▾ + + " +`; + +exports[`Store owners list should drill through interleaved intermediate components: 3: components owned by 1`] = ` +" ▾ + + ▾ " +`; + exports[`Store owners list should drill through intermediate components: 1: mount 1`] = ` [root] ▾ ▾ ▾ - + `; exports[`Store owners list should drill through intermediate components: 2: components owned by 1`] = ` " ▾ ▾ - " + " `; exports[`Store owners list should drill through intermediate components: 3: components owned by 1`] = ` " ▾ ▾ " `; + +exports[`Store owners list should show the proper owners list order and contents after insertions and deletions: 1: mount 1`] = ` +[root] + ▾ + ▾ + ▾ + +`; + +exports[`Store owners list should show the proper owners list order and contents after insertions and deletions: 2: components owned by 1`] = ` +" ▾ + ▾ + " +`; + +exports[`Store owners list should show the proper owners list order and contents after insertions and deletions: 3: update to add direct 1`] = ` +[root] + ▾ + + ▾ + ▾ + +`; + +exports[`Store owners list should show the proper owners list order and contents after insertions and deletions: 4: components owned by 1`] = ` +" ▾ + + ▾ + " +`; + +exports[`Store owners list should show the proper owners list order and contents after insertions and deletions: 5: update to remove indirect 1`] = ` +[root] + ▾ + +`; + +exports[`Store owners list should show the proper owners list order and contents after insertions and deletions: 6: components owned by 1`] = ` +" ▾ + " +`; + +exports[`Store owners list should show the proper owners list order and contents after insertions and deletions: 7: update to remove both 1`] = ` +[root] + +`; + +exports[`Store owners list should show the proper owners list order and contents after insertions and deletions: 8: components owned by 1`] = `" "`; + +exports[`Store owners list should show the proper owners list ordering after reordered children: 1: mount (ascending) 1`] = ` +[root] + ▾ + + + +`; + +exports[`Store owners list should show the proper owners list ordering after reordered children: 2: components owned by 1`] = ` +" ▾ + + + " +`; + +exports[`Store owners list should show the proper owners list ordering after reordered children: 3: update (descending) 1`] = ` +[root] + ▾ + + + +`; + +exports[`Store owners list should show the proper owners list ordering after reordered children: 4: components owned by 1`] = ` +" ▾ + + + " +`; diff --git a/src/__tests__/storeOwners-test.js b/src/__tests__/storeOwners-test.js index 7e49bc803f..f4853b2fe0 100644 --- a/src/__tests__/storeOwners-test.js +++ b/src/__tests__/storeOwners-test.js @@ -27,7 +27,9 @@ describe('Store owners list', () => { it('should drill through intermediate components', () => { const Root = () => ( - +
+ +
); const Wrapper = ({ children }) => children; @@ -47,4 +49,123 @@ describe('Store owners list', () => { printOwnersList(store.getOwnersListForElement(intermediateID)) ).toMatchSnapshot('3: components owned by '); }); + + it('should drill through interleaved intermediate components', () => { + const Root = () => [ + + + , + , + ]; + const Wrapper = ({ children }) => children; + const Leaf = () =>
Leaf
; + const Intermediate = ({ children }) => [ + , + {children}, + ]; + + act(() => ReactDOM.render(, document.createElement('div'))); + expect(store).toMatchSnapshot('1: mount'); + + const rootID = store.getElementIDAtIndex(0); + expect( + printOwnersList(store.getOwnersListForElement(rootID)) + ).toMatchSnapshot('2: components owned by '); + + const intermediateID = store.getElementIDAtIndex(1); + expect( + printOwnersList(store.getOwnersListForElement(intermediateID)) + ).toMatchSnapshot('3: components owned by '); + }); + + it('should show the proper owners list order and contents after insertions and deletions', () => { + const Root = ({ includeDirect, includeIndirect }) => ( +
+ {includeDirect ? : null} + {includeIndirect ? ( + + + + ) : null} +
+ ); + const Wrapper = ({ children }) => children; + const Leaf = () =>
Leaf
; + const Intermediate = ({ children }) => {children}; + + const container = document.createElement('div'); + + act(() => + ReactDOM.render( + , + container + ) + ); + expect(store).toMatchSnapshot('1: mount'); + + const rootID = store.getElementIDAtIndex(0); + expect( + printOwnersList(store.getOwnersListForElement(rootID)) + ).toMatchSnapshot('2: components owned by '); + + act(() => + ReactDOM.render( + , + container + ) + ); + expect(store).toMatchSnapshot('3: update to add direct'); + + expect( + printOwnersList(store.getOwnersListForElement(rootID)) + ).toMatchSnapshot('4: components owned by '); + + act(() => + ReactDOM.render( + , + container + ) + ); + expect(store).toMatchSnapshot('5: update to remove indirect'); + + expect( + printOwnersList(store.getOwnersListForElement(rootID)) + ).toMatchSnapshot('6: components owned by '); + + act(() => + ReactDOM.render( + , + container + ) + ); + expect(store).toMatchSnapshot('7: update to remove both'); + + expect( + printOwnersList(store.getOwnersListForElement(rootID)) + ).toMatchSnapshot('8: components owned by '); + }); + + it('should show the proper owners list ordering after reordered children', () => { + const Root = ({ ascending }) => + ascending + ? [, , ] + : [, , ]; + const Leaf = () =>
Leaf
; + + const container = document.createElement('div'); + act(() => ReactDOM.render(, container)); + expect(store).toMatchSnapshot('1: mount (ascending)'); + + const rootID = store.getElementIDAtIndex(0); + expect( + printOwnersList(store.getOwnersListForElement(rootID)) + ).toMatchSnapshot('2: components owned by '); + + act(() => ReactDOM.render(, container)); + expect(store).toMatchSnapshot('3: update (descending)'); + + expect( + printOwnersList(store.getOwnersListForElement(rootID)) + ).toMatchSnapshot('4: components owned by '); + }); }); diff --git a/src/devtools/store.js b/src/devtools/store.js index 16a447c98c..dcb894160a 100644 --- a/src/devtools/store.js +++ b/src/devtools/store.js @@ -81,6 +81,10 @@ export default class Store extends EventEmitter { // When profiling is in progress, operations are stored so that we can later reconstruct past commit trees. _isProfiling: boolean = false; + // Map of element (id) to the set of elements (ids) it owns. + // This map enables getOwnersListForElement() to avoid traversing the entire tree. + _ownersMap: Map> = new Map(); + // Suspense cache for reading profiling data. _profilingCache: ProfilingCache; @@ -406,10 +410,65 @@ export default class Store extends EventEmitter { return index; } - getOwnersListForElement(id: number): Array { + getOwnersListForElement(ownerID: number): Array { const list = []; + let element = this._idToElement.get(ownerID); + if (element != null) { + list.push({ + ...element, + depth: 0, + }); - this._populateOwnersList(id, id, 0, list); + const unsortedIDs = this._ownersMap.get(ownerID); + if (unsortedIDs !== undefined) { + const depthMap: Map = new Map([[ownerID, 0]]); + + // Items in a set are ordered based on insertion. + // This does not correlate with their order in the tree. + // So first we need to order them. + // I wish we could avoid this sorting operation; we could sort at insertion time, + // but then we'd have to pay sorting costs even if the owners list was never used. + // Seems better to defer the cost, since the set of ids is probably pretty small. + const sortedIDs = Array.from(unsortedIDs).sort( + (idA, idB) => + ((this.getIndexOfElementID(idA): any): number) - + ((this.getIndexOfElementID(idB): any): number) + ); + + // Next we need to determine the appropriate depth for each element in the list. + // The depth in the list may not correspond to the depth in the tree, + // because the list has been filtered to remove intermediate components. + // Perhaps the easiest way to do this is to walk up the tree until we reach either: + // (1) another node that's already in the tree, or (2) the root (owner) + // at which point, our depth is just the depth of that node plus one. + sortedIDs.forEach(id => { + const element = this._idToElement.get(id); + if (element != null) { + let parentID = element.parentID; + + let depth = 0; + while (parentID > 0) { + if (parentID === ownerID || unsortedIDs.has(parentID)) { + depth = depthMap.get(parentID) + 1; + depthMap.set(id, depth); + break; + } + const parent = this._idToElement.get(parentID); + if (parent == null) { + break; + } + parentID = parent.parentID; + } + + if (depth === 0) { + throw Error('Invalid owners list'); + } + + list.push({ ...element, depth }); + } + }); + } + } return list; } @@ -561,32 +620,6 @@ export default class Store extends EventEmitter { THROTTLE_CAPTURE_SCREENSHOT_DURATION ); - _populateOwnersList( - id: number, - ownerID: number, - depth: number, - list: Array - ) { - const element = this._idToElement.get(id); - if (element != null) { - const isInList = id === ownerID || element.ownerID === ownerID; - if (isInList) { - list.push({ - ...element, - depth: depth, - }); - } - element.children.forEach(childID => - this._populateOwnersList( - childID, - ownerID, - isInList ? depth + 1 : depth, - list - ) - ); - } - } - _takeProfilingSnapshotRecursive = (id: number) => { const element = this.getElementByID(id); if (element !== null) { @@ -779,6 +812,15 @@ export default class Store extends EventEmitter { this._idToElement.set(id, element); addedElementIDs.push(id); this._adjustParentTreeWeight(parentElement, 1); + + if (ownerID > 0) { + let set = this._ownersMap.get(ownerID); + if (set === undefined) { + set = new Set(); + this._ownersMap.set(ownerID, set); + } + set.add(id); + } } break; } @@ -798,13 +840,13 @@ export default class Store extends EventEmitter { i = i + 1; const element = ((this._idToElement.get(id): any): Element); - if (element.children.length > 0) { + const { children, ownerID, parentID, weight } = element; + if (children.length > 0) { throw new Error(`Node ${id} was removed before its children.`); } this._idToElement.delete(id); - const parentID = element.parentID; let parentElement = null; if (parentID === 0) { if (__DEBUG__) { @@ -830,8 +872,16 @@ export default class Store extends EventEmitter { parentElement.children.splice(index, 1); } - this._adjustParentTreeWeight(parentElement, -element.weight); + this._adjustParentTreeWeight(parentElement, -weight); removedElementIDs.set(id, parentID); + + this._ownersMap.delete(id); + if (ownerID > 0) { + const set = this._ownersMap.get(ownerID); + if (set !== undefined) { + set.delete(id); + } + } } break; }