From b474ef24e055df51d2f151a2ccef142f21a9a3dc Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Thu, 30 Apr 2020 06:30:04 -0700 Subject: [PATCH] expand components slowly Summary: This diff introduces the concept of `shouldExpand` in DataInspector. Rather than expanding components right away, we only expand if the CPU is idly, this makes sure our app remains interactive, rather than stalling for 15 seconds as shown in the example query. In the future we could solve the blocking by using react suspense as well, but this solution has the nice benefit that it allows inspecting and interacting with the data right away. Changelog: The JSON inspector in plugins like GraphQL no longer freezes Flipper temporarily when expanding large data sets and will remain interactive during Reviewed By: jknoxville Differential Revision: D21302821 fbshipit-source-id: 6a53858f9062175596dc695c4af172d60422abe7 --- .../data-inspector/DataInspector.tsx | 91 +++++++++++++------ .../__tests__/DataInspector.node.tsx | 51 +++++++++-- 2 files changed, 107 insertions(+), 35 deletions(-) diff --git a/desktop/app/src/ui/components/data-inspector/DataInspector.tsx b/desktop/app/src/ui/components/data-inspector/DataInspector.tsx index 0cbe8850e..358e1dc96 100644 --- a/desktop/app/src/ui/components/data-inspector/DataInspector.tsx +++ b/desktop/app/src/ui/components/data-inspector/DataInspector.tsx @@ -22,6 +22,7 @@ import {clipboard} from 'electron'; import deepEqual from 'deep-equal'; import React from 'react'; import {TooltipOptions} from '../TooltipProvider'; +import {shallowEqual} from 'react-redux'; export {DataValueExtractor} from './DataPreview'; @@ -266,16 +267,7 @@ const diffMetadataExtractor: DiffMetadataExtractor = ( return Object.prototype.hasOwnProperty.call(data, key) ? [{data: val}] : []; }; -function isComponentExpanded( - data: any, - diffType: string, - diffValue: any, - isExpanded: boolean, -) { - if (isExpanded) { - return true; - } - +function isComponentExpanded(data: any, diffType: string, diffValue: any) { if (diffValue == null) { return false; } @@ -299,6 +291,7 @@ function isComponentExpanded( } type DataInspectorState = { + shouldExpand: boolean; isExpanded: boolean; isExpandable: boolean; res: any; @@ -327,12 +320,14 @@ export default class DataInspector extends Component< ancestry: [], }; + expandHandle: any = undefined; interaction: (name: string, data?: any) => void; constructor(props: DataInspectorProps) { super(props); this.interaction = reportInteraction('DataInspector', props.path.join(':')); this.state = { + shouldExpand: false, isExpandable: false, isExpanded: false, res: undefined, @@ -346,7 +341,13 @@ export default class DataInspector extends Component< ); } - shouldComponentUpdate(nextProps: DataInspectorProps) { + shouldComponentUpdate( + nextProps: DataInspectorProps, + nextState: DataInspectorState, + ) { + if (!shallowEqual(nextState, this.state)) { + return true; + } const {props} = this; // check if any expanded paths effect this subtree @@ -382,7 +383,7 @@ export default class DataInspector extends Component< static getDerivedStateFromProps( nextProps: DataInspectorProps, - _currentState: DataInspectorState, + currentState: DataInspectorState, ): DataInspectorState { const {data, depth, diff, expandRoot, path} = nextProps; @@ -391,6 +392,7 @@ export default class DataInspector extends Component< if (!res) { return { + shouldExpand: false, isExpanded: false, isExpandable: false, res, @@ -399,29 +401,35 @@ export default class DataInspector extends Component< } const isExpandable = DataInspector.isExpandable(res.value); - const isExpanded = - isExpandable && - (resDiff != null - ? isComponentExpanded( - res.value, - resDiff.type, - resDiff.value, - expandRoot === true || DataInspector.isExpanded(nextProps, path), - ) - : expandRoot === true || DataInspector.isExpanded(nextProps, path)); + let shouldExpand = false; + if (isExpandable) { + if ( + expandRoot === true || + DataInspector.shouldBeExpanded(nextProps, path) + ) { + shouldExpand = true; + } else if (resDiff) { + shouldExpand = isComponentExpanded( + res.value, + resDiff.type, + resDiff.value, + ); + } + } return { - isExpanded, + isExpanded: currentState.isExpanded, + shouldExpand, isExpandable, res, resDiff, }; } - static isExpanded(props: DataInspectorProps, pathParts: Array) { + static shouldBeExpanded(props: DataInspectorProps, pathParts: Array) { const {expanded} = props; - // if we no expanded object then expand everything + // if we have no expanded object then expand everything if (expanded == null) { return true; } @@ -442,6 +450,33 @@ export default class DataInspector extends Component< return true; } + componentDidMount() { + this.expandIfNeeded(); + } + + componentDidUpdate() { + this.expandIfNeeded(); + } + + componentWillUnmount() { + cancelIdleCallback(this.expandHandle); + } + + expandIfNeeded() { + if (this.state.isExpanded !== this.state.shouldExpand) { + cancelIdleCallback(this.expandHandle); + if (!this.state.shouldExpand) { + this.setState({isExpanded: false}); + } else { + this.expandHandle = requestIdleCallback(() => { + this.setState({ + isExpanded: true, + }); + }); + } + } + } + setExpanded(pathParts: Array, isExpanded: boolean) { const {expanded, onExpanded} = this.props; if (!onExpanded || !expanded) { @@ -457,7 +492,11 @@ export default class DataInspector extends Component< } handleClick = () => { - const isExpanded = DataInspector.isExpanded(this.props, this.props.path); + cancelIdleCallback(this.expandHandle); + const isExpanded = DataInspector.shouldBeExpanded( + this.props, + this.props.path, + ); this.interaction(isExpanded ? 'collapsed' : 'expanded'); this.setExpanded(this.props.path, !isExpanded); }; diff --git a/desktop/app/src/ui/components/data-inspector/__tests__/DataInspector.node.tsx b/desktop/app/src/ui/components/data-inspector/__tests__/DataInspector.node.tsx index 495922d0b..2e7510283 100644 --- a/desktop/app/src/ui/components/data-inspector/__tests__/DataInspector.node.tsx +++ b/desktop/app/src/ui/components/data-inspector/__tests__/DataInspector.node.tsx @@ -8,11 +8,40 @@ */ import * as React from 'react'; -import {render, fireEvent} from '@testing-library/react'; +import {render, fireEvent, waitFor} from '@testing-library/react'; jest.mock('../../../../fb/Logger'); import ManagedDataInspector from '../ManagedDataInspector'; +const mocks = { + requestIdleCallback(fn: Function) { + return setTimeout(fn, 1); + }, + cancelIdleCallback(handle: any) { + clearTimeout(handle); + }, +}; + +beforeAll(() => { + Object.keys(mocks).forEach((key) => { + // @ts-ignore + if (!global[key]) { + // @ts-ignore + global[key] = mocks[key]; + } + }); +}); + +afterAll(() => { + Object.keys(mocks).forEach((key) => { + // @ts-ignore + if (global[key] === mocks[key]) { + // @ts-ignore + delete global[key]; + } + }); +}); + const json = { data: { is: { @@ -26,13 +55,13 @@ const json = { test('changing collapsed property works', async () => { const res = render(); - expect((await res.queryAllByText(/is/)).length).toBe(1); // from expandRoot + expect(await res.findByText(/is/)).toBeTruthy(); // from expandRoot expect((await res.queryAllByText(/cool/)).length).toBe(0); res.rerender( , ); - expect((await res.queryAllByText(/cool/)).length).toBe(1); + await waitFor(() => res.findByText(/cool/)); res.rerender( , @@ -42,24 +71,28 @@ test('changing collapsed property works', async () => { test('can manually collapse properties', async () => { const res = render(); + + await res.findByText(/is/); // previewed as key, like: "data: {is, and}" expect((await res.queryAllByText(/awesomely/)).length).toBe(0); - expect((await res.queryAllByText(/is/)).length).toBe(1); // previewed as key, like: "data: {is, and}" // expand twice fireEvent.click(await res.findByText(/data/)); - expect((await res.queryAllByText(/awesomely/)).length).toBe(1); + await res.findByText(/awesomely/); expect((await res.queryAllByText(/cool/)).length).toBe(0); + fireEvent.click(await res.findByText(/is/)); - expect((await res.queryAllByText(/cool/)).length).toBe(1); + await res.findByText(/cool/); expect((await res.queryAllByText(/json/)).length).toBe(0); // this node is not shown // collapsing everything again fireEvent.click(await res.findByText(/data/)); - expect((await res.queryAllByText(/awesomely/)).length).toBe(0); + await waitFor(() => { + expect(res.queryByText(/awesomely/)).toBeNull(); + }); // expand everything again, expanded paths will have been remembered fireEvent.click(await res.findByText(/data/)); - expect((await res.queryAllByText(/is/)).length).toBe(1); - expect((await res.queryAllByText(/awesomely/)).length).toBe(1); + await res.findByText(/is/); + await res.findByText(/awesomely/); expect((await res.queryAllByText(/json/)).length).toBe(0); });