From 4b64d7c01715b9ed6fc70b757cdf8cdc0b03b503 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 16 Apr 2019 13:59:36 -0700 Subject: [PATCH 1/7] Support configurable node/tree collapsed by default behavior --- .../__snapshots__/store-test.js.snap | 159 ++++++- src/__tests__/store-test.js | 412 +++++++++++++----- src/__tests__/storeStress-test.js | 1 + src/devtools/store.js | 28 +- src/devtools/views/Settings/Settings.css | 5 +- src/devtools/views/Settings/Settings.js | 71 ++- 6 files changed, 523 insertions(+), 153 deletions(-) diff --git a/src/__tests__/__snapshots__/store-test.js.snap b/src/__tests__/__snapshots__/store-test.js.snap index 102a328558..7f135dc641 100644 --- a/src/__tests__/__snapshots__/store-test.js.snap +++ b/src/__tests__/__snapshots__/store-test.js.snap @@ -1,6 +1,6 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`Store should display Suspense nodes properly in various states: 1: loading 1`] = ` +exports[`Store collapseNodesByDefault:false should display Suspense nodes properly in various states: 1: loading 1`] = ` [root] ▾ @@ -8,7 +8,7 @@ exports[`Store should display Suspense nodes properly in various states: 1: load `; -exports[`Store should display Suspense nodes properly in various states: 2: resolved 1`] = ` +exports[`Store collapseNodesByDefault:false should display Suspense nodes properly in various states: 2: resolved 1`] = ` [root] ▾ @@ -16,7 +16,7 @@ exports[`Store should display Suspense nodes properly in various states: 2: reso `; -exports[`Store should filter DOM nodes from the store tree: 1: mount 1`] = ` +exports[`Store collapseNodesByDefault:false should filter DOM nodes from the store tree: 1: mount 1`] = ` [root] ▾ ▾ @@ -25,7 +25,7 @@ exports[`Store should filter DOM nodes from the store tree: 1: mount 1`] = ` `; -exports[`Store should support collapsing parts of the tree: 1: mount 1`] = ` +exports[`Store collapseNodesByDefault:false should support collapsing parts of the tree: 1: mount 1`] = ` [root] ▾ ▾ @@ -36,7 +36,7 @@ exports[`Store should support collapsing parts of the tree: 1: mount 1`] = ` `; -exports[`Store should support collapsing parts of the tree: 2: collapse first Parent 1`] = ` +exports[`Store collapseNodesByDefault:false should support collapsing parts of the tree: 2: collapse first Parent 1`] = ` [root] ▾ ▸ @@ -45,14 +45,14 @@ exports[`Store should support collapsing parts of the tree: 2: collapse first Pa `; -exports[`Store should support collapsing parts of the tree: 3: collapse second Parent 1`] = ` +exports[`Store collapseNodesByDefault:false should support collapsing parts of the tree: 3: collapse second Parent 1`] = ` [root] ▾ ▸ ▸ `; -exports[`Store should support collapsing parts of the tree: 4: expand first Parent 1`] = ` +exports[`Store collapseNodesByDefault:false should support collapsing parts of the tree: 4: expand first Parent 1`] = ` [root] ▾ ▾ @@ -61,12 +61,12 @@ exports[`Store should support collapsing parts of the tree: 4: expand first Pare ▸ `; -exports[`Store should support collapsing parts of the tree: 5: collapse Grandparent 1`] = ` +exports[`Store collapseNodesByDefault:false should support collapsing parts of the tree: 5: collapse Grandparent 1`] = ` [root] ▸ `; -exports[`Store should support collapsing parts of the tree: 6: expand Grandparent 1`] = ` +exports[`Store collapseNodesByDefault:false should support collapsing parts of the tree: 6: expand Grandparent 1`] = ` [root] ▾ ▾ @@ -75,7 +75,7 @@ exports[`Store should support collapsing parts of the tree: 6: expand Grandparen ▸ `; -exports[`Store should support mount and update operations for multiple roots: 1: mount 1`] = ` +exports[`Store collapseNodesByDefault:false should support mount and update operations for multiple roots: 1: mount 1`] = ` [root] ▾ @@ -87,7 +87,7 @@ exports[`Store should support mount and update operations for multiple roots: 1: `; -exports[`Store should support mount and update operations for multiple roots: 2: update 1`] = ` +exports[`Store collapseNodesByDefault:false should support mount and update operations for multiple roots: 2: update 1`] = ` [root] ▾ @@ -99,7 +99,7 @@ exports[`Store should support mount and update operations for multiple roots: 2: `; -exports[`Store should support mount and update operations for multiple roots: 3: unmount B 1`] = ` +exports[`Store collapseNodesByDefault:false should support mount and update operations for multiple roots: 3: unmount B 1`] = ` [root] ▾ @@ -108,9 +108,9 @@ exports[`Store should support mount and update operations for multiple roots: 3: `; -exports[`Store should support mount and update operations for multiple roots: 4: unmount A 1`] = ``; +exports[`Store collapseNodesByDefault:false should support mount and update operations for multiple roots: 4: unmount A 1`] = ``; -exports[`Store should support mount and update operations: 1: mount 1`] = ` +exports[`Store collapseNodesByDefault:false should support mount and update operations: 1: mount 1`] = ` [root] ▾ ▾ @@ -125,7 +125,7 @@ exports[`Store should support mount and update operations: 1: mount 1`] = ` `; -exports[`Store should support mount and update operations: 2: update 1`] = ` +exports[`Store collapseNodesByDefault:false should support mount and update operations: 2: update 1`] = ` [root] ▾ ▾ @@ -136,4 +136,131 @@ exports[`Store should support mount and update operations: 2: update 1`] = ` `; -exports[`Store should support mount and update operations: 3: unmount 1`] = ``; +exports[`Store collapseNodesByDefault:false should support mount and update operations: 3: unmount 1`] = ``; + +exports[`Store collapseNodesByDefault:true should display Suspense nodes properly in various states: 1: loading 1`] = ` +[root] + ▸ +`; + +exports[`Store collapseNodesByDefault:true should display Suspense nodes properly in various states: 2: expand Wrapper and Suspense 1`] = ` +[root] + ▾ + + ▾ + +`; + +exports[`Store collapseNodesByDefault:true should display Suspense nodes properly in various states: 2: resolved 1`] = ` +[root] + ▾ + + ▾ + +`; + +exports[`Store collapseNodesByDefault:true should filter DOM nodes from the store tree: 1: mount 1`] = ` +[root] + ▸ +`; + +exports[`Store collapseNodesByDefault:true should filter DOM nodes from the store tree: 2: expand Grandparent 1`] = ` +[root] + ▾ + ▸ + ▸ +`; + +exports[`Store collapseNodesByDefault:true should filter DOM nodes from the store tree: 3: expand Parent 1`] = ` +[root] + ▾ + ▾ + + ▸ +`; + +exports[`Store collapseNodesByDefault:true should support collapsing parts of the tree: 1: mount 1`] = ` +[root] + ▸ +`; + +exports[`Store collapseNodesByDefault:true should support collapsing parts of the tree: 2: expand Grandparent 1`] = ` +[root] + ▾ + ▸ + ▸ +`; + +exports[`Store collapseNodesByDefault:true should support collapsing parts of the tree: 3: expand first Parent 1`] = ` +[root] + ▾ + ▾ + + + ▸ +`; + +exports[`Store collapseNodesByDefault:true should support collapsing parts of the tree: 4: expand second Parent 1`] = ` +[root] + ▾ + ▾ + + + ▾ + + +`; + +exports[`Store collapseNodesByDefault:true should support collapsing parts of the tree: 5: collapse first Parent 1`] = ` +[root] + ▾ + ▸ + ▾ + + +`; + +exports[`Store collapseNodesByDefault:true should support collapsing parts of the tree: 6: collapse second Parent 1`] = ` +[root] + ▾ + ▸ + ▸ +`; + +exports[`Store collapseNodesByDefault:true should support collapsing parts of the tree: 7: collapse Grandparent 1`] = ` +[root] + ▸ +`; + +exports[`Store collapseNodesByDefault:true should support mount and update operations for multiple roots: 1: mount 1`] = ` +[root] + ▸ +[root] + ▸ +`; + +exports[`Store collapseNodesByDefault:true should support mount and update operations for multiple roots: 2: update 1`] = ` +[root] + ▸ +[root] + ▸ +`; + +exports[`Store collapseNodesByDefault:true should support mount and update operations for multiple roots: 3: unmount B 1`] = ` +[root] + ▸ +`; + +exports[`Store collapseNodesByDefault:true should support mount and update operations for multiple roots: 4: unmount A 1`] = ``; + +exports[`Store collapseNodesByDefault:true should support mount and update operations: 1: mount 1`] = ` +[root] + ▸ +`; + +exports[`Store collapseNodesByDefault:true should support mount and update operations: 2: update 1`] = ` +[root] + ▸ +`; + +exports[`Store collapseNodesByDefault:true should support mount and update operations: 3: unmount 1`] = ``; diff --git a/src/__tests__/store-test.js b/src/__tests__/store-test.js index 82da076f0a..259984b74a 100644 --- a/src/__tests__/store-test.js +++ b/src/__tests__/store-test.js @@ -21,142 +21,324 @@ describe('Store', () => { TestUtils = require('react-dom/test-utils'); }); - it('should support mount and update operations', () => { - const Grandparent = ({ count }) => ( - - - - - ); - const Parent = ({ count }) => - new Array(count).fill(true).map((_, index) => ); - const Child = () =>
Hi!
; - - const container = document.createElement('div'); - - act(() => ReactDOM.render(, container)); - expect(store).toMatchSnapshot('1: mount'); - - act(() => ReactDOM.render(, container)); - expect(store).toMatchSnapshot('2: update'); - - act(() => ReactDOM.unmountComponentAtNode(container)); - expect(store).toMatchSnapshot('3: unmount'); + it('should not allow a root node to be collapsed', () => { + // TODO }); - it('should support mount and update operations for multiple roots', () => { - const Parent = ({ count }) => - new Array(count).fill(true).map((_, index) => ); - const Child = () =>
Hi!
; - - const containerA = document.createElement('div'); - const containerB = document.createElement('div'); - - act(() => { - ReactDOM.render(, containerA); - ReactDOM.render(, containerB); + describe('collapseNodesByDefault:false', () => { + beforeEach(() => { + store.collapseNodesByDefault = false; }); - expect(store).toMatchSnapshot('1: mount'); - act(() => { - ReactDOM.render(, containerA); - ReactDOM.render(, containerB); + it('should support mount and update operations', () => { + const Grandparent = ({ count }) => ( + + + + + ); + const Parent = ({ count }) => + new Array(count).fill(true).map((_, index) => ); + const Child = () =>
Hi!
; + + const container = document.createElement('div'); + + act(() => ReactDOM.render(, container)); + expect(store).toMatchSnapshot('1: mount'); + + act(() => ReactDOM.render(, container)); + expect(store).toMatchSnapshot('2: update'); + + act(() => ReactDOM.unmountComponentAtNode(container)); + expect(store).toMatchSnapshot('3: unmount'); }); - expect(store).toMatchSnapshot('2: update'); - act(() => ReactDOM.unmountComponentAtNode(containerB)); - expect(store).toMatchSnapshot('3: unmount B'); + it('should support mount and update operations for multiple roots', () => { + const Parent = ({ count }) => + new Array(count).fill(true).map((_, index) => ); + const Child = () =>
Hi!
; - act(() => ReactDOM.unmountComponentAtNode(containerA)); - expect(store).toMatchSnapshot('4: unmount A'); - }); + const containerA = document.createElement('div'); + const containerB = document.createElement('div'); - it('should filter DOM nodes from the store tree', () => { - const Grandparent = () => ( -
+ act(() => { + ReactDOM.render(, containerA); + ReactDOM.render(, containerB); + }); + expect(store).toMatchSnapshot('1: mount'); + + act(() => { + ReactDOM.render(, containerA); + ReactDOM.render(, containerB); + }); + expect(store).toMatchSnapshot('2: update'); + + act(() => ReactDOM.unmountComponentAtNode(containerB)); + expect(store).toMatchSnapshot('3: unmount B'); + + act(() => ReactDOM.unmountComponentAtNode(containerA)); + expect(store).toMatchSnapshot('4: unmount A'); + }); + + it('should filter DOM nodes from the store tree', () => { + const Grandparent = () => (
+
+ +
- -
- ); - const Parent = () => ( -
- -
- ); - const Child = () =>
Hi!
; + ); + const Parent = () => ( +
+ +
+ ); + const Child = () =>
Hi!
; - act(() => - ReactDOM.render(, document.createElement('div')) - ); - expect(store).toMatchSnapshot('1: mount'); - }); - - it('should display Suspense nodes properly in various states', () => { - const Loading = () =>
Loading...
; - const SuspendingComponent = () => { - throw new Promise(() => {}); - }; - const Component = () => { - return
Hello
; - }; - const Wrapper = ({ shouldSuspense }) => ( - - - }> - {shouldSuspense ? ( - - ) : ( - - )} - - - ); - - const container = document.createElement('div'); - act(() => ReactDOM.render(, container)); - expect(store).toMatchSnapshot('1: loading'); - - act(() => { - ReactDOM.render(, container); + act(() => + ReactDOM.render( + , + document.createElement('div') + ) + ); + expect(store).toMatchSnapshot('1: mount'); + }); + + it('should display Suspense nodes properly in various states', () => { + const Loading = () =>
Loading...
; + const SuspendingComponent = () => { + throw new Promise(() => {}); + }; + const Component = () => { + return
Hello
; + }; + const Wrapper = ({ shouldSuspense }) => ( + + + }> + {shouldSuspense ? ( + + ) : ( + + )} + + + ); + + const container = document.createElement('div'); + act(() => ReactDOM.render(, container)); + expect(store).toMatchSnapshot('1: loading'); + + act(() => { + ReactDOM.render(, container); + }); + expect(store).toMatchSnapshot('2: resolved'); + }); + + it('should support collapsing parts of the tree', () => { + const Grandparent = ({ count }) => ( + + + + + ); + const Parent = ({ count }) => + new Array(count).fill(true).map((_, index) => ); + const Child = () =>
Hi!
; + + act(() => + ReactDOM.render( + , + document.createElement('div') + ) + ); + expect(store).toMatchSnapshot('1: mount'); + + const grandparentID = store.getElementIDAtIndex(0); + const parentOneID = store.getElementIDAtIndex(1); + const parentTwoID = store.getElementIDAtIndex(4); + + act(() => store.toggleIsCollapsed(parentOneID, true)); + expect(store).toMatchSnapshot('2: collapse first Parent'); + + act(() => store.toggleIsCollapsed(parentTwoID, true)); + expect(store).toMatchSnapshot('3: collapse second Parent'); + + act(() => store.toggleIsCollapsed(parentOneID, false)); + expect(store).toMatchSnapshot('4: expand first Parent'); + + act(() => store.toggleIsCollapsed(grandparentID, true)); + expect(store).toMatchSnapshot('5: collapse Grandparent'); + + act(() => store.toggleIsCollapsed(grandparentID, false)); + expect(store).toMatchSnapshot('6: expand Grandparent'); }); - expect(store).toMatchSnapshot('2: resolved'); }); - it('should support collapsing parts of the tree', () => { - const Grandparent = ({ count }) => ( - - - - - ); - const Parent = ({ count }) => - new Array(count).fill(true).map((_, index) => ); - const Child = () =>
Hi!
; + describe('collapseNodesByDefault:true', () => { + beforeEach(() => { + store.collapseNodesByDefault = true; + }); - act(() => - ReactDOM.render(, document.createElement('div')) - ); - expect(store).toMatchSnapshot('1: mount'); + it('should support mount and update operations', () => { + const Grandparent = ({ count }) => ( + + + + + ); + const Parent = ({ count }) => + new Array(count).fill(true).map((_, index) => ); + const Child = () =>
Hi!
; - const grandparentID = store.getElementIDAtIndex(0); - const parentOneID = store.getElementIDAtIndex(1); - const parentTwoID = store.getElementIDAtIndex(4); + const container = document.createElement('div'); - act(() => store.toggleIsCollapsed(parentOneID, true)); - expect(store).toMatchSnapshot('2: collapse first Parent'); + act(() => ReactDOM.render(, container)); + expect(store).toMatchSnapshot('1: mount'); - act(() => store.toggleIsCollapsed(parentTwoID, true)); - expect(store).toMatchSnapshot('3: collapse second Parent'); + act(() => ReactDOM.render(, container)); + expect(store).toMatchSnapshot('2: update'); - act(() => store.toggleIsCollapsed(parentOneID, false)); - expect(store).toMatchSnapshot('4: expand first Parent'); + act(() => ReactDOM.unmountComponentAtNode(container)); + expect(store).toMatchSnapshot('3: unmount'); + }); - act(() => store.toggleIsCollapsed(grandparentID, true)); - expect(store).toMatchSnapshot('5: collapse Grandparent'); + it('should support mount and update operations for multiple roots', () => { + const Parent = ({ count }) => + new Array(count).fill(true).map((_, index) => ); + const Child = () =>
Hi!
; - act(() => store.toggleIsCollapsed(grandparentID, false)); - expect(store).toMatchSnapshot('6: expand Grandparent'); + const containerA = document.createElement('div'); + const containerB = document.createElement('div'); + + act(() => { + ReactDOM.render(, containerA); + ReactDOM.render(, containerB); + }); + expect(store).toMatchSnapshot('1: mount'); + + act(() => { + ReactDOM.render(, containerA); + ReactDOM.render(, containerB); + }); + expect(store).toMatchSnapshot('2: update'); + + act(() => ReactDOM.unmountComponentAtNode(containerB)); + expect(store).toMatchSnapshot('3: unmount B'); + + act(() => ReactDOM.unmountComponentAtNode(containerA)); + expect(store).toMatchSnapshot('4: unmount A'); + }); + + it('should filter DOM nodes from the store tree', () => { + const Grandparent = () => ( +
+
+ +
+ +
+ ); + const Parent = () => ( +
+ +
+ ); + const Child = () =>
Hi!
; + + act(() => + ReactDOM.render( + , + document.createElement('div') + ) + ); + expect(store).toMatchSnapshot('1: mount'); + + act(() => store.toggleIsCollapsed(store.getElementIDAtIndex(0), false)); + expect(store).toMatchSnapshot('2: expand Grandparent'); + + act(() => store.toggleIsCollapsed(store.getElementIDAtIndex(1), false)); + expect(store).toMatchSnapshot('3: expand Parent'); + }); + + it('should display Suspense nodes properly in various states', () => { + const Loading = () =>
Loading...
; + const SuspendingComponent = () => { + throw new Promise(() => {}); + }; + const Component = () => { + return
Hello
; + }; + const Wrapper = ({ shouldSuspense }) => ( + + + }> + {shouldSuspense ? ( + + ) : ( + + )} + + + ); + + const container = document.createElement('div'); + act(() => ReactDOM.render(, container)); + expect(store).toMatchSnapshot('1: loading'); + + // This test isn't meaningful unless we expand the suspended tree + act(() => store.toggleIsCollapsed(store.getElementIDAtIndex(0), false)); + act(() => store.toggleIsCollapsed(store.getElementIDAtIndex(2), false)); + expect(store).toMatchSnapshot('2: expand Wrapper and Suspense'); + + act(() => { + ReactDOM.render(, container); + }); + expect(store).toMatchSnapshot('2: resolved'); + }); + + it('should support collapsing parts of the tree', () => { + const Grandparent = ({ count }) => ( + + + + + ); + const Parent = ({ count }) => + new Array(count).fill(true).map((_, index) => ); + const Child = () =>
Hi!
; + + act(() => + ReactDOM.render( + , + document.createElement('div') + ) + ); + expect(store).toMatchSnapshot('1: mount'); + + const grandparentID = store.getElementIDAtIndex(0); + + act(() => store.toggleIsCollapsed(grandparentID, false)); + expect(store).toMatchSnapshot('2: expand Grandparent'); + + const parentOneID = store.getElementIDAtIndex(1); + const parentTwoID = store.getElementIDAtIndex(2); + + act(() => store.toggleIsCollapsed(parentOneID, false)); + expect(store).toMatchSnapshot('3: expand first Parent'); + + act(() => store.toggleIsCollapsed(parentTwoID, false)); + expect(store).toMatchSnapshot('4: expand second Parent'); + + act(() => store.toggleIsCollapsed(parentOneID, true)); + expect(store).toMatchSnapshot('5: collapse first Parent'); + + act(() => store.toggleIsCollapsed(parentTwoID, true)); + expect(store).toMatchSnapshot('6: collapse second Parent'); + + act(() => store.toggleIsCollapsed(grandparentID, true)); + expect(store).toMatchSnapshot('7: collapse Grandparent'); + }); }); }); diff --git a/src/__tests__/storeStress-test.js b/src/__tests__/storeStress-test.js index ae13007cca..12e111adf5 100644 --- a/src/__tests__/storeStress-test.js +++ b/src/__tests__/storeStress-test.js @@ -18,6 +18,7 @@ describe('StoreStress', () => { beforeEach(() => { bridge = global.bridge; store = global.store; + store.collapseNodesByDefault = false; React = require('react'); ReactDOM = require('react-dom'); diff --git a/src/devtools/store.js b/src/devtools/store.js index 63d87f2544..759e6bfc17 100644 --- a/src/devtools/store.js +++ b/src/devtools/store.js @@ -36,6 +36,8 @@ const debug = (methodName, ...args) => { const LOCAL_STORAGE_CAPTURE_SCREENSHOTS_KEY = 'React::DevTools::captureScreenshots'; +const LOCAL_STORAGE_COLLAPSE_ROOTS_BY_DEFAULT_KEY = + 'React::DevTools::collapseNodesByDefault'; const THROTTLE_CAPTURE_SCREENSHOT_DURATION = 500; @@ -61,6 +63,9 @@ export default class Store extends EventEmitter { _captureScreenshots: boolean = false; + // Should new nodes be collapsed by default when added to the tree? + _collapseNodesByDefault: boolean = true; + // At least one of the injected renderers contains (DEV only) owner metadata. _hasOwnerMetadata: boolean = false; @@ -123,6 +128,11 @@ export default class Store extends EventEmitter { debug('constructor', 'subscribing to Bridge'); } + // Default this setting to true unless otherwise specified. + this._collapseNodesByDefault = + localStorage.getItem(LOCAL_STORAGE_COLLAPSE_ROOTS_BY_DEFAULT_KEY) !== + 'false'; + if (config != null) { const { isProfiling, @@ -178,6 +188,20 @@ export default class Store extends EventEmitter { this.emit('captureScreenshots'); } + get collapseNodesByDefault(): boolean { + return this._collapseNodesByDefault; + } + set collapseNodesByDefault(value: boolean): void { + this._collapseNodesByDefault = value; + + localStorage.setItem( + LOCAL_STORAGE_COLLAPSE_ROOTS_BY_DEFAULT_KEY, + value ? 'true' : 'false' + ); + + this.emit('collapseNodesByDefault'); + } + get hasOwnerMetadata(): boolean { return this._hasOwnerMetadata; } @@ -552,7 +576,7 @@ export default class Store extends EventEmitter { depth: -1, displayName: null, id, - isCollapsed: false, + isCollapsed: false, // Never collapse roots key: null, ownerID: 0, parentID: 0, @@ -601,7 +625,7 @@ export default class Store extends EventEmitter { depth: parentElement.depth + 1, displayName, id, - isCollapsed: false, + isCollapsed: this._collapseNodesByDefault, key, ownerID, parentID: parentElement.id, diff --git a/src/devtools/views/Settings/Settings.css b/src/devtools/views/Settings/Settings.css index 01afd5a23d..7dc9755ca2 100644 --- a/src/devtools/views/Settings/Settings.css +++ b/src/devtools/views/Settings/Settings.css @@ -14,13 +14,14 @@ .Section { display: flex; - flex-direction: column; + flex-direction: row; + align-items: center; margin-right: 0.5rem; margin-bottom: 0.5rem; } .Header { - margin-bottom: 0.5rem; + margin-right: 0.5rem; font-size: var(--font-size-sans-large); } diff --git a/src/devtools/views/Settings/Settings.js b/src/devtools/views/Settings/Settings.js index 1725aa87d2..1c1ed43bcf 100644 --- a/src/devtools/views/Settings/Settings.js +++ b/src/devtools/views/Settings/Settings.js @@ -15,7 +15,7 @@ function Settings(_: {||}) { SettingsContext ); - const subscription = useMemo( + const captureScreenshotsSubscription = useMemo( () => ({ getCurrentValue: () => store.captureScreenshots, subscribe: (callback: Function) => { @@ -25,7 +25,23 @@ function Settings(_: {||}) { }), [store] ); - const captureScreenshots = useSubscription(subscription); + const captureScreenshots = useSubscription( + captureScreenshotsSubscription + ); + + const collapseNodesByDefaultSubscription = useMemo( + () => ({ + getCurrentValue: () => store.collapseNodesByDefault, + subscribe: (callback: Function) => { + store.addListener('collapseNodesByDefault', callback); + return () => store.removeListener('collapseNodesByDefault', callback); + }, + }), + [store] + ); + const collapseNodesByDefault = useSubscription( + collapseNodesByDefaultSubscription + ); const updateDisplayDensity = useCallback( ({ currentTarget }) => { @@ -47,6 +63,12 @@ function Settings(_: {||}) { }, [store] ); + const updateCollapseNodesByDefault = useCallback( + ({ currentTarget }) => { + store.collapseNodesByDefault = currentTarget.checked; + }, + [store] + ); return (
@@ -85,6 +107,17 @@ function Settings(_: {||}) {
+
+
Components tree
+ +
Display density
@@ -111,22 +144,24 @@ function Settings(_: {||}) {
{store.supportsCaptureScreenshots && ( -
-
Profiler
- +
+
+
Profiler
+ +
+ {captureScreenshots && ( +
+ Screenshots will be throttled in order to reduce the negative + impact on performance. +
+ )}
)}
From 037bb0034c70d7b4d5be796b1db7042bfe68ab39 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 16 Apr 2019 14:10:06 -0700 Subject: [PATCH 2/7] Throw if root node is collapsed --- src/__tests__/__snapshots__/store-test.js.snap | 5 +++++ src/__tests__/store-test.js | 15 ++++++++++++++- src/devtools/store.js | 6 +++++- 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/__tests__/__snapshots__/store-test.js.snap b/src/__tests__/__snapshots__/store-test.js.snap index 7f135dc641..bb1e82b5db 100644 --- a/src/__tests__/__snapshots__/store-test.js.snap +++ b/src/__tests__/__snapshots__/store-test.js.snap @@ -264,3 +264,8 @@ exports[`Store collapseNodesByDefault:true should support mount and update opera `; exports[`Store collapseNodesByDefault:true should support mount and update operations: 3: unmount 1`] = ``; + +exports[`Store should not allow a root node to be collapsed: 1: mount 1`] = ` +[root] + +`; diff --git a/src/__tests__/store-test.js b/src/__tests__/store-test.js index 259984b74a..fff720322a 100644 --- a/src/__tests__/store-test.js +++ b/src/__tests__/store-test.js @@ -22,7 +22,20 @@ describe('Store', () => { }); it('should not allow a root node to be collapsed', () => { - // TODO + const Component = () =>
Hi
; + + act(() => + ReactDOM.render(, document.createElement('div')) + ); + expect(store).toMatchSnapshot('1: mount'); + + expect(store.roots).toHaveLength(1); + + const rootID = store.roots[0]; + + expect(() => store.toggleIsCollapsed(rootID, true)).toThrow( + 'Root nodes cannot be collapsed' + ); }); describe('collapseNodesByDefault:false', () => { diff --git a/src/devtools/store.js b/src/devtools/store.js index 759e6bfc17..425fe9b8da 100644 --- a/src/devtools/store.js +++ b/src/devtools/store.js @@ -452,6 +452,10 @@ export default class Store extends EventEmitter { toggleIsCollapsed(id: number, isCollapsed: boolean): void { const element = this.getElementByID(id); if (element !== null) { + if (element.type === ElementTypeRoot) { + throw Error('Root nodes cannot be collapsed'); + } + const oldWeight = element.isCollapsed ? 1 : element.weight; element.isCollapsed = isCollapsed; const newWeight = element.isCollapsed ? 1 : element.weight; @@ -576,7 +580,7 @@ export default class Store extends EventEmitter { depth: -1, displayName: null, id, - isCollapsed: false, // Never collapse roots + isCollapsed: false, // Never collapse roots; it would hide the entire tree. key: null, ownerID: 0, parentID: 0, From 178e89927a4f5322311a1f00ca0e771bd7d358ca Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 17 Apr 2019 07:45:33 -0700 Subject: [PATCH 3/7] Fixed some edge cases with collapsed by default. Still some bugs existing. --- .../__snapshots__/store-test.js.snap | 63 +++++++++-- src/__tests__/setupTests.js | 5 +- src/__tests__/store-test.js | 71 +++++++++++-- src/devtools/store.js | 100 ++++++++++++++---- src/devtools/views/Components/TreeContext.js | 7 +- 5 files changed, 201 insertions(+), 45 deletions(-) diff --git a/src/__tests__/__snapshots__/store-test.js.snap b/src/__tests__/__snapshots__/store-test.js.snap index bb1e82b5db..bfa9d217b1 100644 --- a/src/__tests__/__snapshots__/store-test.js.snap +++ b/src/__tests__/__snapshots__/store-test.js.snap @@ -179,19 +179,62 @@ exports[`Store collapseNodesByDefault:true should filter DOM nodes from the stor ▸ `; -exports[`Store collapseNodesByDefault:true should support collapsing parts of the tree: 1: mount 1`] = ` +exports[`Store collapseNodesByDefault:true should support expanding deep parts of the tree: 1: mount 1`] = ` +[root] + ▸ +`; + +exports[`Store collapseNodesByDefault:true should support expanding deep parts of the tree: 2: expand deepest node 1`] = ` +[root] + ▾ + ▾ + ▾ + ▾ + +`; + +exports[`Store collapseNodesByDefault:true should support expanding deep parts of the tree: 3: collapse root 1`] = ` +[root] + ▸ +`; + +exports[`Store collapseNodesByDefault:true should support expanding deep parts of the tree: 4: expand root 1`] = ` +[root] + ▾ + ▾ + ▾ + ▾ + +`; + +exports[`Store collapseNodesByDefault:true should support expanding deep parts of the tree: 5: collapse middle node 1`] = ` +[root] + ▾ + ▸ +`; + +exports[`Store collapseNodesByDefault:true should support expanding deep parts of the tree: 6: expand middle node 1`] = ` +[root] + ▾ + ▾ + ▾ + ▾ + +`; + +exports[`Store collapseNodesByDefault:true should support expanding parts of the tree: 1: mount 1`] = ` [root] ▸ `; -exports[`Store collapseNodesByDefault:true should support collapsing parts of the tree: 2: expand Grandparent 1`] = ` +exports[`Store collapseNodesByDefault:true should support expanding parts of the tree: 2: expand Grandparent 1`] = ` [root] ▾ ▸ ▸ `; -exports[`Store collapseNodesByDefault:true should support collapsing parts of the tree: 3: expand first Parent 1`] = ` +exports[`Store collapseNodesByDefault:true should support expanding parts of the tree: 3: expand first Parent 1`] = ` [root] ▾ ▾ @@ -200,7 +243,7 @@ exports[`Store collapseNodesByDefault:true should support collapsing parts of th ▸ `; -exports[`Store collapseNodesByDefault:true should support collapsing parts of the tree: 4: expand second Parent 1`] = ` +exports[`Store collapseNodesByDefault:true should support expanding parts of the tree: 4: expand second Parent 1`] = ` [root] ▾ ▾ @@ -211,7 +254,7 @@ exports[`Store collapseNodesByDefault:true should support collapsing parts of th `; -exports[`Store collapseNodesByDefault:true should support collapsing parts of the tree: 5: collapse first Parent 1`] = ` +exports[`Store collapseNodesByDefault:true should support expanding parts of the tree: 5: collapse first Parent 1`] = ` [root] ▾ ▸ @@ -220,14 +263,14 @@ exports[`Store collapseNodesByDefault:true should support collapsing parts of th `; -exports[`Store collapseNodesByDefault:true should support collapsing parts of the tree: 6: collapse second Parent 1`] = ` +exports[`Store collapseNodesByDefault:true should support expanding parts of the tree: 6: collapse second Parent 1`] = ` [root] ▾ ▸ ▸ `; -exports[`Store collapseNodesByDefault:true should support collapsing parts of the tree: 7: collapse Grandparent 1`] = ` +exports[`Store collapseNodesByDefault:true should support expanding parts of the tree: 7: collapse Grandparent 1`] = ` [root] ▸ `; @@ -255,12 +298,14 @@ exports[`Store collapseNodesByDefault:true should support mount and update opera exports[`Store collapseNodesByDefault:true should support mount and update operations: 1: mount 1`] = ` [root] - ▸ + ▸ + ▸ `; exports[`Store collapseNodesByDefault:true should support mount and update operations: 2: update 1`] = ` [root] - ▸ + ▸ + ▸ `; exports[`Store collapseNodesByDefault:true should support mount and update operations: 3: unmount 1`] = ``; diff --git a/src/__tests__/setupTests.js b/src/__tests__/setupTests.js index fefe83b258..40475c8245 100644 --- a/src/__tests__/setupTests.js +++ b/src/__tests__/setupTests.js @@ -31,8 +31,11 @@ env.beforeEach(() => { const agent = new Agent(); agent.addBridge(bridge); - initBackend(global.__REACT_DEVTOOLS_GLOBAL_HOOK__, agent, global); + const hook = global.__REACT_DEVTOOLS_GLOBAL_HOOK__; + initBackend(hook, agent, global); + + global.agent = agent; global.bridge = bridge; global.store = new Store(bridge); }); diff --git a/src/__tests__/store-test.js b/src/__tests__/store-test.js index fff720322a..c4e6b42497 100644 --- a/src/__tests__/store-test.js +++ b/src/__tests__/store-test.js @@ -195,22 +195,32 @@ describe('Store', () => { }); it('should support mount and update operations', () => { - const Grandparent = ({ count }) => ( - - - - - ); const Parent = ({ count }) => new Array(count).fill(true).map((_, index) => ); const Child = () =>
Hi!
; const container = document.createElement('div'); - act(() => ReactDOM.render(, container)); + act(() => + ReactDOM.render( + + + + , + container + ) + ); expect(store).toMatchSnapshot('1: mount'); - act(() => ReactDOM.render(, container)); + act(() => + ReactDOM.render( + + + + , + container + ) + ); expect(store).toMatchSnapshot('2: update'); act(() => ReactDOM.unmountComponentAtNode(container)); @@ -311,7 +321,7 @@ describe('Store', () => { expect(store).toMatchSnapshot('2: resolved'); }); - it('should support collapsing parts of the tree', () => { + it('should support expanding parts of the tree', () => { const Grandparent = ({ count }) => ( @@ -353,5 +363,48 @@ describe('Store', () => { act(() => store.toggleIsCollapsed(grandparentID, true)); expect(store).toMatchSnapshot('7: collapse Grandparent'); }); + + it('should support expanding deep parts of the tree', () => { + const Wrapper = ({ forwardedRef }) => ( + + ); + const Nested = ({ depth, forwardedRef }) => + depth > 0 ? ( + + ) : ( +
+ ); + + const ref = React.createRef(); + + act(() => + ReactDOM.render( + , + document.createElement('div') + ) + ); + expect(store).toMatchSnapshot('1: mount'); + + const deepestedNodeID = global.agent.getIDForNode(ref.current); + + act(() => store.toggleIsCollapsed(deepestedNodeID, false)); + expect(store).toMatchSnapshot('2: expand deepest node'); + + const rootID = store.getElementIDAtIndex(0); + + act(() => store.toggleIsCollapsed(rootID, true)); + expect(store).toMatchSnapshot('3: collapse root'); + + act(() => store.toggleIsCollapsed(rootID, false)); + expect(store).toMatchSnapshot('4: expand root'); + + const id = store.getElementIDAtIndex(1); + + act(() => store.toggleIsCollapsed(id, true)); + expect(store).toMatchSnapshot('5: collapse middle node'); + + act(() => store.toggleIsCollapsed(id, false)); + expect(store).toMatchSnapshot('6: expand middle node'); + }); }); }); diff --git a/src/devtools/store.js b/src/devtools/store.js index 425fe9b8da..2451abd98f 100644 --- a/src/devtools/store.js +++ b/src/devtools/store.js @@ -315,9 +315,8 @@ export default class Store extends EventEmitter { // Find the element in the tree using the weight of each node... // Skip over the root itself, because roots aren't visible in the Elements tree. - const firstChildID = ((root: any): Element).children[0]; - let currentElement = ((this._idToElement.get(firstChildID): any): Element); - let currentWeight = rootWeight; + let currentElement = ((root: any): Element); + let currentWeight = rootWeight - 1; while (index !== currentWeight) { const numChildren = currentElement.children.length; for (let i = 0; i < numChildren; i++) { @@ -449,25 +448,66 @@ export default class Store extends EventEmitter { // We do this to avoid mismatches on e.g. CommitTreeBuilder that would cause errors. } + // TODO Maybe split this into two methods: expand() and collapse() toggleIsCollapsed(id: number, isCollapsed: boolean): void { const element = this.getElementByID(id); if (element !== null) { - if (element.type === ElementTypeRoot) { - throw Error('Root nodes cannot be collapsed'); + if (isCollapsed) { + if (element.type === ElementTypeRoot) { + throw Error('Root nodes cannot be collapsed'); + } + + if (element.isCollapsed) { + return; + } + + element.isCollapsed = true; + + const weightDelta = 1 - element.weight; + + let parentElement = ((this._idToElement.get( + element.parentID + ): any): Element); + while (parentElement != null) { + parentElement.weight += weightDelta; + parentElement = this._idToElement.get(parentElement.parentID); + } + } else { + let currentElement = element; + while (currentElement != null) { + const oldWeight = currentElement.isCollapsed + ? 1 + : currentElement.weight; + currentElement.isCollapsed = false; + const newWeight = currentElement.isCollapsed + ? 1 + : currentElement.weight; + const weightDelta = newWeight - oldWeight; + + let parentElement = ((this._idToElement.get( + currentElement.parentID + ): any): Element); + while (parentElement != null) { + parentElement.weight += weightDelta; + if (parentElement.isCollapsed) { + break; + } + parentElement = this._idToElement.get(parentElement.parentID); + } + + currentElement = + currentElement.parentID !== 0 + ? this.getElementByID(currentElement.parentID) + : null; + } } - const oldWeight = element.isCollapsed ? 1 : element.weight; - element.isCollapsed = isCollapsed; - const newWeight = element.isCollapsed ? 1 : element.weight; - const weightDelta = newWeight - oldWeight; - - this._weightAcrossRoots += weightDelta; - - let parentElement = this._idToElement.get(element.parentID); - while (parentElement != null) { - parentElement.weight += weightDelta; - parentElement = this._idToElement.get(parentElement.parentID); - } + let weightAcrossRoots = 0; + this._roots.forEach(rootID => { + const { weight } = ((this.getElementByID(rootID): any): Element); + weightAcrossRoots += weight; + }); + this._weightAcrossRoots = weightAcrossRoots; // The Tree context's search reducer expects an explicit list of ids for nodes that were added or removed. // In this case, we can pass it empty arrays since nodes in a collapsed tree are still there (just hidden). @@ -867,19 +907,23 @@ export default class Store extends EventEmitter { // Used for Jest snapshot testing. // May also be useful for visually debugging the tree, so it lives on the Store. - __toSnapshot = () => { + __toSnapshot = (includeWeight: boolean = false) => { const snapshotLines = []; let rootWeight = 0; this._roots.forEach(rootID => { - snapshotLines.push('[root]'); - 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(`No element for index ${i}`); + } + let prefix = ' '; if (element.children.length > 0) { prefix = element.isCollapsed ? '▸' : '▾'; @@ -890,15 +934,29 @@ export default class Store extends EventEmitter { 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}>` + '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/TreeContext.js b/src/devtools/views/Components/TreeContext.js index 0a10c4c959..0be84a9e1d 100644 --- a/src/devtools/views/Components/TreeContext.js +++ b/src/devtools/views/Components/TreeContext.js @@ -748,11 +748,8 @@ function TreeContextController({ children, viewElementSource }: Props) { if (state.selectedElementID !== null) { let element = store.getElementByID(state.selectedElementID); - while (element !== null && element.parentID > 0) { - element = ((store.getElementByID(element.parentID): any): Element); - if (element.isCollapsed) { - store.toggleIsCollapsed(element.id, false); - } + if (element !== null && element.parentID > 0) { + store.toggleIsCollapsed(element.parentID, false); } } } From 74be38912464d63020eb52df016624b9d227335c Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 17 Apr 2019 09:39:54 -0700 Subject: [PATCH 4/7] Fixed a bug with re-ordering of children within a collapsed node --- .../__snapshots__/store-test.js.snap | 30 +++++++++++++ src/__tests__/store-test.js | 42 +++++++++++++++++++ src/devtools/store.js | 24 +++++++---- 3 files changed, 88 insertions(+), 8 deletions(-) diff --git a/src/__tests__/__snapshots__/store-test.js.snap b/src/__tests__/__snapshots__/store-test.js.snap index bfa9d217b1..fe37abce49 100644 --- a/src/__tests__/__snapshots__/store-test.js.snap +++ b/src/__tests__/__snapshots__/store-test.js.snap @@ -138,6 +138,26 @@ exports[`Store collapseNodesByDefault:false should support mount and update oper exports[`Store collapseNodesByDefault:false should support mount and update operations: 3: unmount 1`] = ``; +exports[`Store collapseNodesByDefault:false should support reordering of children: 1: mount 1`] = ` +[root] + ▾ + ▾ + + ▾ + + +`; + +exports[`Store collapseNodesByDefault:false should support reordering of children: 3: reorder children 1`] = ` +[root] + ▾ + ▾ + + + ▾ + +`; + exports[`Store collapseNodesByDefault:true should display Suspense nodes properly in various states: 1: loading 1`] = ` [root] ▸ @@ -310,6 +330,16 @@ exports[`Store collapseNodesByDefault:true should support mount and update opera exports[`Store collapseNodesByDefault:true should support mount and update operations: 3: unmount 1`] = ``; +exports[`Store collapseNodesByDefault:true should support reordering of children: 1: mount 1`] = ` +[root] + ▸ +`; + +exports[`Store collapseNodesByDefault:true should support reordering of children: 3: reorder children 1`] = ` +[root] + ▸ +`; + exports[`Store should not allow a root node to be collapsed: 1: mount 1`] = ` [root] diff --git a/src/__tests__/store-test.js b/src/__tests__/store-test.js index c4e6b42497..bfe0d88da8 100644 --- a/src/__tests__/store-test.js +++ b/src/__tests__/store-test.js @@ -187,6 +187,27 @@ describe('Store', () => { act(() => store.toggleIsCollapsed(grandparentID, false)); expect(store).toMatchSnapshot('6: expand Grandparent'); }); + + it('should support reordering of children', () => { + const Component = ({ children = null }) => children; + + const Foo = () => []; + const Bar = () => [, ]; + const foo = ; + const bar = ; + + const container = document.createElement('div'); + + act(() => + ReactDOM.render({[foo, bar]}, container) + ); + expect(store).toMatchSnapshot('1: mount'); + + act(() => + ReactDOM.render({[bar, foo]}, container) + ); + expect(store).toMatchSnapshot('3: reorder children'); + }); }); describe('collapseNodesByDefault:true', () => { @@ -406,5 +427,26 @@ describe('Store', () => { act(() => store.toggleIsCollapsed(id, false)); expect(store).toMatchSnapshot('6: expand middle node'); }); + + it('should support reordering of children', () => { + const Component = ({ children = null }) => children; + + const Foo = () => []; + const Bar = () => [, ]; + const foo = ; + const bar = ; + + const container = document.createElement('div'); + + act(() => + ReactDOM.render({[foo, bar]}, container) + ); + expect(store).toMatchSnapshot('1: mount'); + + act(() => + ReactDOM.render({[bar, foo]}, container) + ); + expect(store).toMatchSnapshot('3: reorder children'); + }); }); }); diff --git a/src/devtools/store.js b/src/devtools/store.js index 2451abd98f..817af3daf0 100644 --- a/src/devtools/store.js +++ b/src/devtools/store.js @@ -544,6 +544,7 @@ export default class Store extends EventEmitter { } if (__DEBUG__) { + console.groupCollapsed('onBridgeOperations'); debug('onBridgeOperations', operations); } @@ -797,17 +798,19 @@ export default class Store extends EventEmitter { element = ((this._idToElement.get(id): any): Element); element.children = Array.from(children); - const prevWeight = element.weight; - let childWeight = 0; + if (!element.isCollapsed) { + const prevWeight = element.weight; + let childWeight = 0; - children.forEach(childID => { - const child = ((this._idToElement.get(childID): any): Element); - childWeight += child.weight; - }); + children.forEach(childID => { + const child = ((this._idToElement.get(childID): any): Element); + childWeight += child.weight; + }); - element.weight = childWeight + 1; + element.weight = childWeight + 1; - weightDelta = childWeight + 1 - prevWeight; + weightDelta = childWeight + 1 - prevWeight; + } break; case TREE_OPERATION_UPDATE_TREE_BASE_DURATION: // Base duration updates are only sent while profiling is in progress. @@ -861,6 +864,11 @@ export default class Store extends EventEmitter { this.emit('roots'); } + if (__DEBUG__) { + console.log(this.__toSnapshot(true)); + console.groupEnd(); + } + this.emit('mutated', [addedElementIDs, removedElementIDs]); }; From c107e03132638324011b920273df6f19ff70ff45 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 17 Apr 2019 09:45:03 -0700 Subject: [PATCH 5/7] Flow fix --- .../__snapshots__/store-test.js.snap | 8 +++---- src/__tests__/store-test.js | 22 +++++++------------ 2 files changed, 12 insertions(+), 18 deletions(-) diff --git a/src/__tests__/__snapshots__/store-test.js.snap b/src/__tests__/__snapshots__/store-test.js.snap index fe37abce49..78585b906b 100644 --- a/src/__tests__/__snapshots__/store-test.js.snap +++ b/src/__tests__/__snapshots__/store-test.js.snap @@ -140,7 +140,7 @@ exports[`Store collapseNodesByDefault:false should support mount and update oper exports[`Store collapseNodesByDefault:false should support reordering of children: 1: mount 1`] = ` [root] - ▾ + ▾ ▾ ▾ @@ -150,7 +150,7 @@ exports[`Store collapseNodesByDefault:false should support reordering of childre exports[`Store collapseNodesByDefault:false should support reordering of children: 3: reorder children 1`] = ` [root] - ▾ + ▾ ▾ @@ -332,12 +332,12 @@ exports[`Store collapseNodesByDefault:true should support mount and update opera exports[`Store collapseNodesByDefault:true should support reordering of children: 1: mount 1`] = ` [root] - ▸ + ▸ `; exports[`Store collapseNodesByDefault:true should support reordering of children: 3: reorder children 1`] = ` [root] - ▸ + ▸ `; exports[`Store should not allow a root node to be collapsed: 1: mount 1`] = ` diff --git a/src/__tests__/store-test.js b/src/__tests__/store-test.js index bfe0d88da8..d25a9b1a65 100644 --- a/src/__tests__/store-test.js +++ b/src/__tests__/store-test.js @@ -189,7 +189,8 @@ describe('Store', () => { }); it('should support reordering of children', () => { - const Component = ({ children = null }) => children; + const Root = ({ children }) => children; + const Component = () => null; const Foo = () => []; const Bar = () => [, ]; @@ -198,14 +199,10 @@ describe('Store', () => { const container = document.createElement('div'); - act(() => - ReactDOM.render({[foo, bar]}, container) - ); + act(() => ReactDOM.render({[foo, bar]}, container)); expect(store).toMatchSnapshot('1: mount'); - act(() => - ReactDOM.render({[bar, foo]}, container) - ); + act(() => ReactDOM.render({[bar, foo]}, container)); expect(store).toMatchSnapshot('3: reorder children'); }); }); @@ -429,7 +426,8 @@ describe('Store', () => { }); it('should support reordering of children', () => { - const Component = ({ children = null }) => children; + const Root = ({ children }) => children; + const Component = () => null; const Foo = () => []; const Bar = () => [, ]; @@ -438,14 +436,10 @@ describe('Store', () => { const container = document.createElement('div'); - act(() => - ReactDOM.render({[foo, bar]}, container) - ); + act(() => ReactDOM.render({[foo, bar]}, container)); expect(store).toMatchSnapshot('1: mount'); - act(() => - ReactDOM.render({[bar, foo]}, container) - ); + act(() => ReactDOM.render({[bar, foo]}, container)); expect(store).toMatchSnapshot('3: reorder children'); }); }); From 634d4fece2f800d95104eaa4cedf62b751f2a3d9 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 17 Apr 2019 13:03:18 -0700 Subject: [PATCH 6/7] Added more inline comments --- src/devtools/store.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/devtools/store.js b/src/devtools/store.js index 817af3daf0..99ba142fca 100644 --- a/src/devtools/store.js +++ b/src/devtools/store.js @@ -458,6 +458,8 @@ export default class Store extends EventEmitter { } if (element.isCollapsed) { + // There's nothing to change in this case. + // We can exit early (without even emiting a "mutated" event). return; } @@ -469,6 +471,8 @@ export default class Store extends EventEmitter { element.parentID ): any): Element); while (parentElement != null) { + // We don't need to break on a collapsed parent in the same way as the expand case below. + // That's because collapsing a node doesn't "bubble" and affect its parents. parentElement.weight += weightDelta; parentElement = this._idToElement.get(parentElement.parentID); } @@ -490,6 +494,9 @@ export default class Store extends EventEmitter { while (parentElement != null) { parentElement.weight += weightDelta; if (parentElement.isCollapsed) { + // It's important to break on a collapsed parent when expanding nodes. + // That's because expanding a node "bubbles" up and expands all parents as well. + // Breaking in this case prevents us from over-incrementing the expanded weights. break; } parentElement = this._idToElement.get(parentElement.parentID); From 75e1e1431b34f68c50f1971e20662540d285eeb6 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 17 Apr 2019 13:11:18 -0700 Subject: [PATCH 7/7] Expanded the reorder tests slightly --- .../__snapshots__/store-test.js.snap | 37 +++++++++++++++++++ src/__tests__/store-test.js | 18 +++++++++ 2 files changed, 55 insertions(+) diff --git a/src/__tests__/__snapshots__/store-test.js.snap b/src/__tests__/__snapshots__/store-test.js.snap index 78585b906b..1e1af1d669 100644 --- a/src/__tests__/__snapshots__/store-test.js.snap +++ b/src/__tests__/__snapshots__/store-test.js.snap @@ -158,6 +158,21 @@ exports[`Store collapseNodesByDefault:false should support reordering of childre `; +exports[`Store collapseNodesByDefault:false should support reordering of children: 4: collapse root 1`] = ` +[root] + ▸ +`; + +exports[`Store collapseNodesByDefault:false should support reordering of children: 5: expand root 1`] = ` +[root] + ▾ + ▾ + + + ▾ + +`; + exports[`Store collapseNodesByDefault:true should display Suspense nodes properly in various states: 1: loading 1`] = ` [root] ▸ @@ -340,6 +355,28 @@ exports[`Store collapseNodesByDefault:true should support reordering of children ▸ `; +exports[`Store collapseNodesByDefault:true should support reordering of children: 4: expand root 1`] = ` +[root] + ▾ + ▸ + ▸ +`; + +exports[`Store collapseNodesByDefault:true should support reordering of children: 5: expand leaves 1`] = ` +[root] + ▾ + ▾ + + + ▾ + +`; + +exports[`Store collapseNodesByDefault:true should support reordering of children: 6: collapse root 1`] = ` +[root] + ▸ +`; + exports[`Store should not allow a root node to be collapsed: 1: mount 1`] = ` [root] diff --git a/src/__tests__/store-test.js b/src/__tests__/store-test.js index d25a9b1a65..5dad4ed956 100644 --- a/src/__tests__/store-test.js +++ b/src/__tests__/store-test.js @@ -204,6 +204,12 @@ describe('Store', () => { act(() => ReactDOM.render({[bar, foo]}, container)); expect(store).toMatchSnapshot('3: reorder children'); + + act(() => store.toggleIsCollapsed(store.getElementIDAtIndex(0), true)); + expect(store).toMatchSnapshot('4: collapse root'); + + act(() => store.toggleIsCollapsed(store.getElementIDAtIndex(0), false)); + expect(store).toMatchSnapshot('5: expand root'); }); }); @@ -441,6 +447,18 @@ describe('Store', () => { act(() => ReactDOM.render({[bar, foo]}, container)); expect(store).toMatchSnapshot('3: reorder children'); + + act(() => store.toggleIsCollapsed(store.getElementIDAtIndex(0), false)); + expect(store).toMatchSnapshot('4: expand root'); + + act(() => { + store.toggleIsCollapsed(store.getElementIDAtIndex(2), false); + store.toggleIsCollapsed(store.getElementIDAtIndex(1), false); + }); + expect(store).toMatchSnapshot('5: expand leaves'); + + act(() => store.toggleIsCollapsed(store.getElementIDAtIndex(0), true)); + expect(store).toMatchSnapshot('6: collapse root'); }); }); });