From 15eacae02c1d526bdc54cbff6e32d068fd483257 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 24 Apr 2019 15:19:13 -0700 Subject: [PATCH] Moved owners list calculations into the store and added tests This is being done to fix a drill-through bug, although the initial fix is perhaps not the most performant one. At least we have test coverage now and a temporary fix. --- .../__snapshots__/storeOwners-test.js.snap | 20 ++++ src/__tests__/storeOwners-test.js | 50 ++++++++++ src/__tests__/storeSerializer.js | 73 ++++++++++++++- src/devtools/store.js | 92 ++++++++----------- src/devtools/views/Components/Element.js | 13 +-- src/devtools/views/Components/Tree.js | 4 - src/devtools/views/Components/TreeContext.js | 58 +++--------- 7 files changed, 192 insertions(+), 118 deletions(-) create mode 100644 src/__tests__/__snapshots__/storeOwners-test.js.snap create mode 100644 src/__tests__/storeOwners-test.js diff --git a/src/__tests__/__snapshots__/storeOwners-test.js.snap b/src/__tests__/__snapshots__/storeOwners-test.js.snap new file mode 100644 index 0000000000..cbb2177c9b --- /dev/null +++ b/src/__tests__/__snapshots__/storeOwners-test.js.snap @@ -0,0 +1,20 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +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`] = ` +" ▾ + ▾ " +`; diff --git a/src/__tests__/storeOwners-test.js b/src/__tests__/storeOwners-test.js new file mode 100644 index 0000000000..7e49bc803f --- /dev/null +++ b/src/__tests__/storeOwners-test.js @@ -0,0 +1,50 @@ +// @flow + +const { printOwnersList } = require('./storeSerializer'); + +describe('Store owners list', () => { + let React; + let ReactDOM; + let TestUtils; + let store; + + const act = (callback: Function) => { + TestUtils.act(() => { + callback(); + }); + jest.runAllTimers(); // Flush Bridge operations + }; + + beforeEach(() => { + store = global.store; + store.collapseNodesByDefault = false; + + React = require('react'); + ReactDOM = require('react-dom'); + TestUtils = require('react-dom/test-utils'); + }); + + it('should drill through 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 '); + }); +}); diff --git a/src/__tests__/storeSerializer.js b/src/__tests__/storeSerializer.js index 0f4b8602fc..fc1b8d18ff 100644 --- a/src/__tests__/storeSerializer.js +++ b/src/__tests__/storeSerializer.js @@ -1,9 +1,74 @@ import Store from 'src/devtools/store'; -export function test(value) { - return value instanceof Store; +// test() is part of Jest's serializer API +export function test(maybeStore) { + return maybeStore instanceof Store; } -export function print(value, serialize, indent) { - return value.__toSnapshot(); +// print() is part of Jest's serializer API +export function print(store, serialize, indent) { + return printStore(store); +} + +export function printElement(element, includeWeight = false) { + let prefix = ' '; + if (element.children.length > 0) { + prefix = element.isCollapsed ? '▸' : '▾'; + } + + let key = ''; + if (element.key !== null) { + key = ` key="${element.key}"`; + } + + let suffix = ''; + if (includeWeight) { + suffix = ` (${element.isCollapsed ? 1 : element.weight})`; + } + + return `${' '.repeat(element.depth + 1)}${prefix} <${element.displayName || + 'null'}${key}>${suffix}`; +} + +export function printOwnersList(elements, includeWeight = false) { + return elements + .map(element => printElement(element, includeWeight)) + .join('\n'); +} + +// Used for Jest snapshot testing. +// May also be useful for visually debugging the tree, so it lives on the Store. +export function printStore(store, includeWeight = false) { + const snapshotLines = []; + + let rootWeight = 0; + + store.roots.forEach(rootID => { + const { weight } = store.getElementByID(rootID); + + snapshotLines.push('[root]' + (includeWeight ? ` (${weight})` : '')); + + for (let i = rootWeight; i < rootWeight + weight; i++) { + const element = store.getElementAtIndex(i); + + if (element == null) { + throw Error(`Could not find element at index ${i}`); + } + + snapshotLines.push(printElement(element, includeWeight)); + } + + rootWeight += weight; + }); + + // Make sure the pretty-printed test align with the Store's reported number of total rows. + if (rootWeight !== store.numElements) { + throw Error( + `Inconsistent Store state. Individual root weights (${rootWeight}) do not match total weight (${ + store.numElements + })` + ); + } + + return snapshotLines.join('\n'); } diff --git a/src/devtools/store.js b/src/devtools/store.js index 60a99ed4b2..16a447c98c 100644 --- a/src/devtools/store.js +++ b/src/devtools/store.js @@ -13,6 +13,7 @@ import { ElementTypeRoot } from './types'; import { utfDecodeString } from '../utils'; import { __DEBUG__ } from '../constants'; import ProfilingCache from './ProfilingCache'; +import { printStore } from 'src/__tests__/storeSerializer'; import type { ElementType } from './types'; import type { Element } from './views/Components/types'; @@ -405,6 +406,14 @@ export default class Store extends EventEmitter { return index; } + getOwnersListForElement(id: number): Array { + const list = []; + + this._populateOwnersList(id, id, 0, list); + + return list; + } + getRendererIDForElement(id: number): number | null { let current = this._idToElement.get(id); while (current != null) { @@ -552,6 +561,32 @@ 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) { @@ -870,7 +905,7 @@ export default class Store extends EventEmitter { } if (__DEBUG__) { - console.log(this.__toSnapshot(true)); + console.log(printStore(this, true)); console.groupEnd(); } @@ -917,59 +952,4 @@ export default class Store extends EventEmitter { this._bridge.removeListener('profilingStatus', this.onProfilingStatus); this._bridge.removeListener('shutdown', this.onBridgeShutdown); }; - - // Used for Jest snapshot testing. - // May also be useful for visually debugging the tree, so it lives on the Store. - __toSnapshot = (includeWeight: boolean = false) => { - const snapshotLines = []; - - let rootWeight = 0; - - this._roots.forEach(rootID => { - const { weight } = ((this.getElementByID(rootID): any): Element); - - snapshotLines.push('[root]' + (includeWeight ? ` (${weight})` : '')); - - for (let i = rootWeight; i < rootWeight + weight; i++) { - const element = ((this.getElementAtIndex(i): any): Element); - - if (element == null) { - throw Error(`Could not find element at index ${i}`); - } - - let prefix = ' '; - if (element.children.length > 0) { - prefix = element.isCollapsed ? '▸' : '▾'; - } - - let key = ''; - if (element.key !== null) { - key = ` key="${element.key}"`; - } - - let suffix = ''; - if (includeWeight) { - suffix = ` (${element.isCollapsed ? 1 : element.weight})`; - } - - snapshotLines.push( - `${' '.repeat(element.depth + 1)}${prefix} <${element.displayName || - 'null'}${key}>${suffix}` - ); - } - - rootWeight += weight; - }); - - // Make sure the pretty-printed test align with the Store's reported number of total rows. - if (rootWeight !== this._weightAcrossRoots) { - throw Error( - `Inconsistent Store state. Individual root weights (${rootWeight}) do not match total weight (${ - this._weightAcrossRoots - })` - ); - } - - return snapshotLines.join('\n'); - }; } diff --git a/src/devtools/views/Components/Element.js b/src/devtools/views/Components/Element.js index 0fa93b40ac..bfe0ce65ce 100644 --- a/src/devtools/views/Components/Element.js +++ b/src/devtools/views/Components/Element.js @@ -29,17 +29,14 @@ type Props = { export default function ElementView({ data, index, style }: Props) { const store = useContext(StoreContext); - const { - baseDepth, - ownerFlatTree, - ownerStack, - selectedElementID, - } = useContext(TreeStateContext); + const { ownerFlatTree, ownerStack, selectedElementID } = useContext( + TreeStateContext + ); const dispatch = useContext(TreeDispatcherContext); const element = ownerFlatTree !== null - ? store.getElementByID(ownerFlatTree[index]) + ? ownerFlatTree[index] : store.getElementAtIndex(index); const [isHovered, setIsHovered] = useState(false); @@ -158,7 +155,7 @@ export default function ElementView({ data, index, style }: Props) { ...style, // "style" comes from react-window // Left padding presents the appearance of a nested tree structure. - paddingLeft: `${(depth - baseDepth) * 0.75 + 0.25}rem`, + paddingLeft: `${depth * 0.75 + 0.25}rem`, // These style overrides enable the background color to fill the full visible width, // when combined with the CSS tweaks in Tree. diff --git a/src/devtools/views/Components/Tree.js b/src/devtools/views/Components/Tree.js index 9ecfa58e60..0a96f163b0 100644 --- a/src/devtools/views/Components/Tree.js +++ b/src/devtools/views/Components/Tree.js @@ -22,7 +22,6 @@ import SearchInput from './SearchInput'; import styles from './Tree.css'; export type ItemData = {| - baseDepth: number, numElements: number, isNavigatingWithKeyboard: boolean, lastScrolledIDRef: { current: number | null }, @@ -35,7 +34,6 @@ type Props = {||}; export default function Tree(props: Props) { const dispatch = useContext(TreeDispatcherContext); const { - baseDepth, numElements, ownerStack, searchIndex, @@ -255,7 +253,6 @@ export default function Tree(props: Props) { // This includes the owner context, since it controls a filtered view of the tree. const itemData = useMemo( () => ({ - baseDepth, numElements, isNavigatingWithKeyboard, onElementMouseEnter: handleElementMouseEnter, @@ -263,7 +260,6 @@ export default function Tree(props: Props) { treeFocused, }), [ - baseDepth, numElements, isNavigatingWithKeyboard, handleElementMouseEnter, diff --git a/src/devtools/views/Components/TreeContext.js b/src/devtools/views/Components/TreeContext.js index 160dc44eaf..002f6b619e 100644 --- a/src/devtools/views/Components/TreeContext.js +++ b/src/devtools/views/Components/TreeContext.js @@ -40,7 +40,6 @@ import type { Element } from './types'; type StateContext = {| // Tree - baseDepth: number, numElements: number, selectedElementID: number | null, selectedElementIndex: number | null, @@ -51,7 +50,7 @@ type StateContext = {| searchText: string, // Owners - ownerFlatTree: Array | null, + ownerFlatTree: Array | null, ownerStack: Array, ownerStackIndex: number | null, @@ -133,7 +132,6 @@ TreeDispatcherContext.displayName = 'TreeDispatcherContext'; type State = {| // Tree - baseDepth: number, numElements: number, selectedElementID: number | null, selectedElementIndex: number | null, @@ -146,7 +144,7 @@ type State = {| // Owners ownerStack: Array, ownerStackIndex: number | null, - ownerFlatTree: Array | null, + ownerFlatTree: Array | null, // Inspection element panel inspectedElementID: number | null, @@ -441,7 +439,6 @@ function reduceSearchState(store: Store, state: State, action: Action): State { function reduceOwnersState(store: Store, state: State, action: Action): State { let { - baseDepth, numElements, selectedElementID, selectedElementIndex, @@ -477,7 +474,9 @@ function reduceOwnersState(store: Store, state: State, action: Action): State { } if (selectedElementID !== null && ownerFlatTree !== null) { // Mutation might have caused the index of this ID to shift. - selectedElementIndex = ownerFlatTree.indexOf(selectedElementID); + selectedElementIndex = ownerFlatTree.findIndex( + element => element.id === selectedElementID + ); } } else { if (selectedElementID !== null) { @@ -509,7 +508,9 @@ function reduceOwnersState(store: Store, state: State, action: Action): State { if (ownerFlatTree !== null) { const payload = (action: ACTION_SELECT_ELEMENT_BY_ID).payload; selectedElementIndex = - payload === null ? null : ownerFlatTree.indexOf(payload); + payload === null + ? null + : ownerFlatTree.findIndex(element => element.id === payload); } break; case 'SELECT_NEXT_ELEMENT_IN_TREE': @@ -574,18 +575,11 @@ function reduceOwnersState(store: Store, state: State, action: Action): State { ) { if (ownerStackIndex === null) { ownerFlatTree = null; - baseDepth = 0; numElements = store.numElements; } else { - ownerFlatTree = calculateCurrentOwnerList( - store, - ownerStack[ownerStackIndex], - ownerStack[ownerStackIndex], - [] + ownerFlatTree = store.getOwnersListForElement( + ownerStack[ownerStackIndex] ); - - baseDepth = ((store.getElementByID(ownerFlatTree[0]): any): Element) - .depth; numElements = ownerFlatTree.length; } } @@ -595,14 +589,14 @@ function reduceOwnersState(store: Store, state: State, action: Action): State { if (selectedElementIndex === null) { selectedElementID = null; } else if (ownerFlatTree !== null) { - selectedElementID = ownerFlatTree[((selectedElementIndex: any): number)]; + selectedElementID = + ownerFlatTree[((selectedElementIndex: any): number)].id; } } return { ...state, - baseDepth, numElements, selectedElementID, selectedElementIndex, @@ -692,7 +686,6 @@ function TreeContextController({ children }: Props) { const [state, dispatch] = useReducer(reducer, { // Tree - baseDepth: 0, numElements: store.numElements, selectedElementIndex: null, selectedElementID: null, @@ -783,33 +776,6 @@ function TreeContextController({ children }: Props) { ); } -function calculateCurrentOwnerList( - store: Store, - rootOwnerID: number, - elementID: number, - ownerList: Array -): Array { - if (elementID === rootOwnerID) { - ownerList.push(elementID); - const { children } = ((store.getElementByID(elementID): any): Element); - children.forEach(childID => - calculateCurrentOwnerList(store, rootOwnerID, childID, ownerList) - ); - } else { - const { children, ownerID } = ((store.getElementByID( - elementID - ): any): Element); - if (ownerID === rootOwnerID) { - ownerList.push(elementID); - children.forEach(childID => - calculateCurrentOwnerList(store, rootOwnerID, childID, ownerList) - ); - } - } - - return ownerList; -} - function recursivelySearchTree( store: Store, elementID: number,