From 09b023a6e6f0d9a3eed95d488d6e910f305e4ad8 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 19 Apr 2019 14:54:28 -0700 Subject: [PATCH 01/18] Naive suspense implementation for selected element panel This commit leaves a few major things uunresolved: * We aren't yet polling for updates * We aren't yet using the two setState pattern * The resource cache will grow unbounded over time because we aren't yet clearing items from it * The renderer interface is not smart enough to avoid resending unchanged data between requests --- src/backend/renderer.js | 1 + src/devtools/InspectedElementCache.js | 93 ++++++++++++++++++ src/devtools/ProfilingCache.js | 4 + src/devtools/cache.js | 60 +++++++----- src/devtools/store.js | 11 ++- src/devtools/views/Components/Components.css | 10 ++ src/devtools/views/Components/Components.js | 10 +- .../views/Components/SelectedElement.js | 94 ++----------------- src/devtools/views/Components/TreeContext.js | 4 + 9 files changed, 175 insertions(+), 112 deletions(-) create mode 100644 src/devtools/InspectedElementCache.js diff --git a/src/backend/renderer.js b/src/backend/renderer.js index e41ea782ff..ce607379ce 100644 --- a/src/backend/renderer.js +++ b/src/backend/renderer.js @@ -1492,6 +1492,7 @@ export function attach( } } + // TODO Send a no-op message if the specified Fiber hasn't been committed since it was last inspected. function inspectElementRaw(id: number): InspectedElement | null { let fiber = idToFiberMap.get(id); diff --git a/src/devtools/InspectedElementCache.js b/src/devtools/InspectedElementCache.js new file mode 100644 index 0000000000..424589dd21 --- /dev/null +++ b/src/devtools/InspectedElementCache.js @@ -0,0 +1,93 @@ +// @flow + +import EventEmitter from 'events'; +import { createResource } from './cache'; +import Store from './store'; +import { hydrate } from 'src/hydration'; + +import type { + DehydratedData, + InspectedElement, +} from 'src/devtools/views/Components/types'; +import type { Resource } from './cache'; +import type { Bridge } from '../types'; + +type ResolveFn = (inspectedElement: InspectedElement) => void; + +type Params = {| + id: number, + rendererID: number, +|}; + +// TODO Use an LRU for the underlying caching mechanism, to prevent memory leaks. + +// TODO Something needs to poll for (unprompted) updates. + +export default class InspectedElementCache extends EventEmitter { + _bridge: Bridge; + _store: Store; + + _pendingRequests: Map = new Map(); + + _resource: Resource = createResource( + ({ id, rendererID }: Params) => { + return new Promise(resolve => { + this._pendingRequests.set(id, resolve); + this._bridge.send('inspectElement', { + id, + rendererID, + }); + }); + }, + ({ id, rendererID }: Params) => id + ); + + constructor(bridge: Bridge, store: Store) { + super(); + + this._bridge = bridge; + this._store = store; + + bridge.addListener('inspectedElement', this._onInspectedElement); + } + + read(id: number): InspectedElement | null { + const rendererID = this._store.getRendererIDForElement(id); + + if (rendererID != null) { + return this._resource.read({ id, rendererID }); + } else { + return null; + } + } + + _onInspectedElement = (inspectedElement: InspectedElement) => { + const id = inspectedElement.id; + + if (inspectedElement != null) { + inspectedElement.context = hydrateHelper(inspectedElement.context); + inspectedElement.hooks = hydrateHelper(inspectedElement.hooks); + inspectedElement.props = hydrateHelper(inspectedElement.props); + inspectedElement.state = hydrateHelper(inspectedElement.state); + } + + const resolveFn = this._pendingRequests.get(id); + if (resolveFn != null) { + this._pendingRequests.delete(id); + + resolveFn(inspectedElement); + } else { + this._resource.write(id, inspectedElement); + + this.emit('invalidated', id); + } + }; +} + +function hydrateHelper(dehydratedData: DehydratedData | null): Object | null { + if (dehydratedData !== null) { + return hydrate(dehydratedData.data, dehydratedData.cleaned); + } else { + return null; + } +} diff --git a/src/devtools/ProfilingCache.js b/src/devtools/ProfilingCache.js index e0b4cfabc9..7dd8b3d946 100644 --- a/src/devtools/ProfilingCache.js +++ b/src/devtools/ProfilingCache.js @@ -92,6 +92,7 @@ export default class ProfilingCache { CommitDetails: Resource< CommitDetailsParams, + string, CommitDetailsFrontend > = createResource( ({ commitIndex, rendererID, rootID }: CommitDetailsParams) => { @@ -136,6 +137,7 @@ export default class ProfilingCache { FiberCommits: Resource< FiberCommitsParams, + string, FiberCommitsFrontend > = createResource( ({ fiberID, rendererID, rootID }: FiberCommitsParams) => { @@ -168,6 +170,7 @@ export default class ProfilingCache { Interactions: Resource< InteractionsParams, + number, InteractionsFrontend > = createResource( ({ rendererID, rootID }: InteractionsParams) => { @@ -198,6 +201,7 @@ export default class ProfilingCache { ProfilingSummary: Resource< ProfilingSummaryParams, + number, ProfilingSummaryFrontend > = createResource( ({ rendererID, rootID }: ProfilingSummaryParams) => { diff --git a/src/devtools/cache.js b/src/devtools/cache.js index 58577adf32..a30fb7d20c 100644 --- a/src/devtools/cache.js +++ b/src/devtools/cache.js @@ -25,9 +25,9 @@ type PendingResult = {| value: Suspender, |}; -type ResolvedResult = {| +type ResolvedResult = {| status: 1, - value: V, + value: Value, |}; type RejectedResult = {| @@ -35,11 +35,13 @@ type RejectedResult = {| value: mixed, |}; -type Result = PendingResult | ResolvedResult | RejectedResult; +type Result = PendingResult | ResolvedResult | RejectedResult; -export type Resource = { - read(I): V, - preload(I): void, +export type Resource = { + invalidate(Key): void, + read(Input): Value, + preload(Input): void, + write(Key, Value): void, }; const Pending = 0; @@ -67,14 +69,14 @@ function identityHashFn(input) { const CacheContext = createContext(null); -const entries: Map, Map> = new Map(); +const entries: Map, Map> = new Map(); -function accessResult( +function accessResult( resource: any, - fetch: I => Thenable, - input: I, - key: K -): Result { + fetch: Input => Thenable, + input: Input, + key: Key +): Result { let entriesForResource = entries.get(resource); if (entriesForResource === undefined) { entriesForResource = new Map(); @@ -86,7 +88,7 @@ function accessResult( thenable.then( value => { if (newResult.status === Pending) { - const resolvedResult: ResolvedResult = (newResult: any); + const resolvedResult: ResolvedResult = (newResult: any); resolvedResult.status = Resolved; resolvedResult.value = value; } @@ -110,21 +112,28 @@ function accessResult( } } -export function createResource( - fetch: I => Thenable, - maybeHashInput?: I => K -): Resource { - const hashInput: I => K = +export function createResource( + fetch: Input => Thenable, + maybeHashInput?: Input => Key +): Resource { + const hashInput: Input => Key = maybeHashInput !== undefined ? maybeHashInput : (identityHashFn: any); const resource = { - read(input: I): V { + invalidate(key: Key): void { + const entriesForResource = entries.get(resource); + if (entriesForResource !== undefined) { + entriesForResource.delete(key); + } + }, + + read(input: Input): Value { // Prevent access outside of render. // eslint-disable-next-line react-hooks/rules-of-hooks readContext(CacheContext); const key = hashInput(input); - const result: Result = accessResult(resource, fetch, input, key); + const result: Result = accessResult(resource, fetch, input, key); switch (result.status) { case Pending: { const suspender = result.value; @@ -144,7 +153,7 @@ export function createResource( } }, - preload(input: I): void { + preload(input: Input): void { // Prevent access outside of render. // eslint-disable-next-line react-hooks/rules-of-hooks readContext(CacheContext); @@ -152,6 +161,15 @@ export function createResource( const key = hashInput(input); accessResult(resource, fetch, input, key); }, + + write(key: Key, value: Value): void { + let entriesForResource = entries.get(resource); + if (entriesForResource === undefined) { + entriesForResource = new Map(); + entries.set(resource, entriesForResource); + } + entriesForResource.set(key, value); + }, }; return resource; } diff --git a/src/devtools/store.js b/src/devtools/store.js index a59c9c9fa4..8692e29b65 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 InspectedElementCache from './InspectedElementCache'; import type { ElementType } from './types'; import type { Element } from './views/Components/types'; @@ -75,11 +76,14 @@ export default class Store extends EventEmitter { // The user has imported a previously exported profiling session. _importedProfilingData: ImportedProfilingData | null = null; + // Suspense cache for lazy-loaded inspected Element data. + _inspectedElementCache: InspectedElementCache; + // The backend is currently profiling. // When profiling is in progress, operations are stored so that we can later reconstruct past commit trees. _isProfiling: boolean = false; - // Suspense cache for reading profilign data. + // Suspense cache for reading profiling data. _profilingCache: ProfilingCache; // Map of root (id) to a list of tree mutation that occur during profiling. @@ -170,6 +174,7 @@ export default class Store extends EventEmitter { // so the frontend needs to ask the backend for its status after mounting. bridge.send('getProfilingStatus'); + this._inspectedElementCache = new InspectedElementCache(bridge, this); this._profilingCache = new ProfilingCache(bridge, this); } @@ -224,6 +229,10 @@ export default class Store extends EventEmitter { this.emit('importedProfilingData'); } + get inspectedElementCache(): InspectedElementCache { + return this._inspectedElementCache; + } + get isProfiling(): boolean { return this._isProfiling; } diff --git a/src/devtools/views/Components/Components.css b/src/devtools/views/Components/Components.css index 6523b5e515..775b1faef1 100644 --- a/src/devtools/views/Components/Components.css +++ b/src/devtools/views/Components/Components.css @@ -24,3 +24,13 @@ flex-direction: column; } } + +.Loading { + height: 100%; + padding-left: 0.5rem; + display: flex; + align-items: center; + justify-content: center; + font-size: var(--font-size-sans-large); + color: var(--color-dim); +} diff --git a/src/devtools/views/Components/Components.js b/src/devtools/views/Components/Components.js index f38830baab..bad008d0bc 100644 --- a/src/devtools/views/Components/Components.js +++ b/src/devtools/views/Components/Components.js @@ -1,6 +1,6 @@ // @flow -import React from 'react'; +import React, { Suspense } from 'react'; import Tree from './Tree'; import SelectedElement from './SelectedElement'; import styles from './Components.css'; @@ -14,10 +14,16 @@ function Components(_: {||}) {
- + }> + +
); } +function Loading() { + return
Loading...
; +} + export default portaledContent(Components); diff --git a/src/devtools/views/Components/SelectedElement.js b/src/devtools/views/Components/SelectedElement.js index d5048a142e..359e59b333 100644 --- a/src/devtools/views/Components/SelectedElement.js +++ b/src/devtools/views/Components/SelectedElement.js @@ -1,19 +1,12 @@ // @flow -import React, { - useCallback, - useContext, - useEffect, - useRef, - useState, -} from 'react'; +import React, { useCallback, useContext } from 'react'; import { TreeContext } from './TreeContext'; import { BridgeContext, StoreContext } from '../context'; import Button from '../Button'; import ButtonIcon from '../ButtonIcon'; import HooksTree from './HooksTree'; import InspectedElementTree from './InspectedElementTree'; -import { hydrate } from 'src/hydration'; import styles from './SelectedElement.css'; import { ElementTypeClass, @@ -23,8 +16,7 @@ import { ElementTypeSuspense, } from '../../types'; -import type { InspectedElement } from './types'; -import type { DehydratedData, Element } from './types'; +import type { Element, InspectedElement } from './types'; export type Props = {||}; @@ -36,7 +28,10 @@ export default function SelectedElement(_: Props) { const element = selectedElementID !== null ? store.getElementByID(selectedElementID) : null; - const inspectedElement = useInspectedElement(selectedElementID); + const inspectedElement = + selectedElementID != null + ? store.inspectedElementCache.read(selectedElementID) + : null; const highlightElement = useCallback(() => { if (element !== null && selectedElementID !== null) { @@ -261,80 +256,3 @@ function OwnerView({ displayName, id }: { displayName: string, id: number }) { ); } - -function hydrateHelper(dehydratedData: DehydratedData | null): Object | null { - if (dehydratedData !== null) { - return hydrate(dehydratedData.data, dehydratedData.cleaned); - } else { - return null; - } -} - -function useInspectedElement(id: number | null): InspectedElement | null { - const idRef = useRef(id); - const bridge = useContext(BridgeContext); - const store = useContext(StoreContext); - - const [inspectedElement, setInspectedElement] = useState(null); - - useEffect(() => { - // Track the current selected element ID. - // We ignore any backend updates about previously selected elements. - idRef.current = id; - - // Hide previous/stale insepected element to avoid temporarily showing the wrong values. - setInspectedElement(null); - - // A null id indicates that there's nothing currently selected in the tree. - if (id === null) { - return () => {}; - } - - const rendererID = store.getRendererIDForElement(id); - - // Update the $r variable. - bridge.send('selectElement', { id, rendererID }); - - // Update props, state, and context in the side panel. - const sendBridgeRequest = () => { - bridge.send('inspectElement', { id, rendererID }); - }; - - let timeoutID = null; - - const onInspectedElement = (inspectedElement: InspectedElement) => { - if (!inspectedElement || inspectedElement.id !== idRef.current) { - // Ignore bridge updates about previously selected elements. - return; - } - - if (inspectedElement !== null) { - inspectedElement.context = hydrateHelper(inspectedElement.context); - inspectedElement.hooks = hydrateHelper(inspectedElement.hooks); - inspectedElement.props = hydrateHelper(inspectedElement.props); - inspectedElement.state = hydrateHelper(inspectedElement.state); - } - - setInspectedElement(inspectedElement); - - // Ask for an update in a second. - // Make sure we only ask once though. - clearTimeout(((timeoutID: any): TimeoutID)); - timeoutID = setTimeout(sendBridgeRequest, 1000); - }; - - bridge.addListener('inspectedElement', onInspectedElement); - - sendBridgeRequest(); - - return () => { - bridge.removeListener('inspectedElement', onInspectedElement); - - if (timeoutID !== null) { - clearTimeout(timeoutID); - } - }; - }, [bridge, id, idRef, store]); - - return inspectedElement; -} diff --git a/src/devtools/views/Components/TreeContext.js b/src/devtools/views/Components/TreeContext.js index bd295c920e..5c8a300e8a 100644 --- a/src/devtools/views/Components/TreeContext.js +++ b/src/devtools/views/Components/TreeContext.js @@ -33,6 +33,10 @@ import Store from '../../store'; import type { Element } from './types'; +// TODO Use two setState pattern for selecting Fibers: +// The first update should be default priority and should select a new element in the Tree. +// The second update should be deferred priority and should trigger suspense. + type Context = {| // Tree baseDepth: number, From c6b19cc1416070e341a7a9be2c160a8472b7577b Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Sat, 20 Apr 2019 08:33:00 -0700 Subject: [PATCH 02/18] Refactored insepected element cache to use the context API --- src/devtools/InspectedElementCache.js | 93 ----------- src/devtools/cache.js | 34 ++-- src/devtools/store.js | 9 -- src/devtools/views/Components/Components.js | 12 +- .../Components/InspectedElementContext.js | 145 ++++++++++++++++++ .../views/Components/SelectedElement.js | 7 +- 6 files changed, 170 insertions(+), 130 deletions(-) delete mode 100644 src/devtools/InspectedElementCache.js create mode 100644 src/devtools/views/Components/InspectedElementContext.js diff --git a/src/devtools/InspectedElementCache.js b/src/devtools/InspectedElementCache.js deleted file mode 100644 index 424589dd21..0000000000 --- a/src/devtools/InspectedElementCache.js +++ /dev/null @@ -1,93 +0,0 @@ -// @flow - -import EventEmitter from 'events'; -import { createResource } from './cache'; -import Store from './store'; -import { hydrate } from 'src/hydration'; - -import type { - DehydratedData, - InspectedElement, -} from 'src/devtools/views/Components/types'; -import type { Resource } from './cache'; -import type { Bridge } from '../types'; - -type ResolveFn = (inspectedElement: InspectedElement) => void; - -type Params = {| - id: number, - rendererID: number, -|}; - -// TODO Use an LRU for the underlying caching mechanism, to prevent memory leaks. - -// TODO Something needs to poll for (unprompted) updates. - -export default class InspectedElementCache extends EventEmitter { - _bridge: Bridge; - _store: Store; - - _pendingRequests: Map = new Map(); - - _resource: Resource = createResource( - ({ id, rendererID }: Params) => { - return new Promise(resolve => { - this._pendingRequests.set(id, resolve); - this._bridge.send('inspectElement', { - id, - rendererID, - }); - }); - }, - ({ id, rendererID }: Params) => id - ); - - constructor(bridge: Bridge, store: Store) { - super(); - - this._bridge = bridge; - this._store = store; - - bridge.addListener('inspectedElement', this._onInspectedElement); - } - - read(id: number): InspectedElement | null { - const rendererID = this._store.getRendererIDForElement(id); - - if (rendererID != null) { - return this._resource.read({ id, rendererID }); - } else { - return null; - } - } - - _onInspectedElement = (inspectedElement: InspectedElement) => { - const id = inspectedElement.id; - - if (inspectedElement != null) { - inspectedElement.context = hydrateHelper(inspectedElement.context); - inspectedElement.hooks = hydrateHelper(inspectedElement.hooks); - inspectedElement.props = hydrateHelper(inspectedElement.props); - inspectedElement.state = hydrateHelper(inspectedElement.state); - } - - const resolveFn = this._pendingRequests.get(id); - if (resolveFn != null) { - this._pendingRequests.delete(id); - - resolveFn(inspectedElement); - } else { - this._resource.write(id, inspectedElement); - - this.emit('invalidated', id); - } - }; -} - -function hydrateHelper(dehydratedData: DehydratedData | null): Object | null { - if (dehydratedData !== null) { - return hydrate(dehydratedData.data, dehydratedData.cleaned); - } else { - return null; - } -} diff --git a/src/devtools/cache.js b/src/devtools/cache.js index a30fb7d20c..11daf5d65f 100644 --- a/src/devtools/cache.js +++ b/src/devtools/cache.js @@ -1,6 +1,7 @@ // @flow import React, { createContext } from 'react'; +import LRU from 'lru-cache'; // Cache implementation was forked from the React repo: // https://github.com/facebook/react/blob/master/packages/react-cache/src/ReactCache.js @@ -63,10 +64,6 @@ function readContext(Context, observedBits) { return dispatcher.readContext(Context, observedBits); } -function identityHashFn(input) { - return input; -} - const CacheContext = createContext(null); const entries: Map, Map> = new Map(); @@ -77,12 +74,8 @@ function accessResult( input: Input, key: Key ): Result { - let entriesForResource = entries.get(resource); - if (entriesForResource === undefined) { - entriesForResource = new Map(); - entries.set(resource, entriesForResource); - } - let entry = entriesForResource.get(key); + const entriesForResource = ((entries.get(resource): any): Map); + const entry = entriesForResource.get(key); if (entry === undefined) { const thenable = fetch(input); thenable.then( @@ -114,16 +107,16 @@ function accessResult( export function createResource( fetch: Input => Thenable, - maybeHashInput?: Input => Key + hashInput: Input => Key, + useLRU?: boolean = false ): Resource { - const hashInput: Input => Key = - maybeHashInput !== undefined ? maybeHashInput : (identityHashFn: any); - const resource = { invalidate(key: Key): void { - const entriesForResource = entries.get(resource); - if (entriesForResource !== undefined) { + const entriesForResource = ((entries.get(resource): any): Map); + if (entriesForResource instanceof Map) { entriesForResource.delete(key); + } else { + entriesForResource.set(key, undefined); } }, @@ -163,14 +156,13 @@ export function createResource( }, write(key: Key, value: Value): void { - let entriesForResource = entries.get(resource); - if (entriesForResource === undefined) { - entriesForResource = new Map(); - entries.set(resource, entriesForResource); - } + const entriesForResource = ((entries.get(resource): any): Map); entriesForResource.set(key, value); }, }; + + entries.set(resource, useLRU ? new LRU({ max: 10 }) : new Map()); + return resource; } diff --git a/src/devtools/store.js b/src/devtools/store.js index 8692e29b65..9b67920c6c 100644 --- a/src/devtools/store.js +++ b/src/devtools/store.js @@ -13,7 +13,6 @@ import { ElementTypeRoot } from './types'; import { utfDecodeString } from '../utils'; import { __DEBUG__ } from '../constants'; import ProfilingCache from './ProfilingCache'; -import InspectedElementCache from './InspectedElementCache'; import type { ElementType } from './types'; import type { Element } from './views/Components/types'; @@ -76,9 +75,6 @@ export default class Store extends EventEmitter { // The user has imported a previously exported profiling session. _importedProfilingData: ImportedProfilingData | null = null; - // Suspense cache for lazy-loaded inspected Element data. - _inspectedElementCache: InspectedElementCache; - // The backend is currently profiling. // When profiling is in progress, operations are stored so that we can later reconstruct past commit trees. _isProfiling: boolean = false; @@ -174,7 +170,6 @@ export default class Store extends EventEmitter { // so the frontend needs to ask the backend for its status after mounting. bridge.send('getProfilingStatus'); - this._inspectedElementCache = new InspectedElementCache(bridge, this); this._profilingCache = new ProfilingCache(bridge, this); } @@ -229,10 +224,6 @@ export default class Store extends EventEmitter { this.emit('importedProfilingData'); } - get inspectedElementCache(): InspectedElementCache { - return this._inspectedElementCache; - } - get isProfiling(): boolean { return this._isProfiling; } diff --git a/src/devtools/views/Components/Components.js b/src/devtools/views/Components/Components.js index bad008d0bc..bce5941af0 100644 --- a/src/devtools/views/Components/Components.js +++ b/src/devtools/views/Components/Components.js @@ -3,9 +3,11 @@ import React, { Suspense } from 'react'; import Tree from './Tree'; import SelectedElement from './SelectedElement'; -import styles from './Components.css'; +import { InspectedElementContextController } from './InspectedElementContext'; import portaledContent from '../portaledContent'; +import styles from './Components.css'; + function Components(_: {||}) { // TODO Flex wrappers below should be user resizable. return ( @@ -14,9 +16,11 @@ function Components(_: {||}) {
- }> - - + + }> + + +
); diff --git a/src/devtools/views/Components/InspectedElementContext.js b/src/devtools/views/Components/InspectedElementContext.js new file mode 100644 index 0000000000..53af90d5a1 --- /dev/null +++ b/src/devtools/views/Components/InspectedElementContext.js @@ -0,0 +1,145 @@ +// @flow + +import React, { + createContext, + useCallback, + useContext, + useEffect, + useMemo, + useState, +} from 'react'; +import { createResource } from '../../cache'; +import { BridgeContext, StoreContext } from '../context'; +import { hydrate } from 'src/hydration'; + +import type { + DehydratedData, + InspectedElement, +} from 'src/devtools/views/Components/types'; +import type { Resource } from '../../cache'; + +// TODO Something needs to poll for (unprompted) updates. + +// TODO The curretn approach caches resources permanently. +// We won't even ask for an update if an element is reselected. +// I think we need to separate the polling for an update from the suspense cache. +// This way we can always resened (and poll on an interval) for the selected id, +// and the cache here can just invalidate itself as responses stream in. + +type Params = {| + id: number, + rendererID: number, +|}; + +type Context = {| + read(id: number): InspectedElement | null, +|}; + +const InspectedElementContext = createContext(((null: any): Context)); +InspectedElementContext.displayName = 'InspectedElementContext'; + +type ResolveFn = (inspectedElement: InspectedElement) => void; +type InProgressRequest = {| + promise: Promise, + resolveFn: ResolveFn, +|}; + +type Props = {| + children: React$Node, +|}; + +function InspectedElementContextController({ children }: Props) { + const bridge = useContext(BridgeContext); + const store = useContext(StoreContext); + + const [count, setCount] = useState(0); + + const inProgressRequests = useMemo>( + () => new Map(), + [] + ); + + const resource = useMemo>( + () => + createResource( + ({ id, rendererID }: Params) => { + let request = inProgressRequests.get(id); + if (request != null) { + return request.promise; + } + + let resolveFn = ((null: any): ResolveFn); + const promise = new Promise(resolve => { + resolveFn = resolve; + + bridge.send('inspectElement', { id, rendererID }); + }); + + inProgressRequests.set(id, { promise, resolveFn }); + + return promise; + }, + ({ id, rendererID }: Params) => id + ), + [bridge, inProgressRequests] + ); + + useEffect(() => { + const onInspectedElement = (inspectedElement: InspectedElement | null) => { + if (inspectedElement != null) { + const id = inspectedElement.id; + + inspectedElement.context = hydrateHelper(inspectedElement.context); + inspectedElement.hooks = hydrateHelper(inspectedElement.hooks); + inspectedElement.props = hydrateHelper(inspectedElement.props); + inspectedElement.state = hydrateHelper(inspectedElement.state); + + const request = inProgressRequests.get(id); + if (request != null) { + inProgressRequests.delete(id); + request.resolveFn(inspectedElement); + } else { + resource.write(id, inspectedElement); + + // Schedule update with React. + setCount(count => count + 1); + } + } + }; + + bridge.addListener('inspectedElement', onInspectedElement); + return () => bridge.removeListener('inspectElement', onInspectedElement); + }, [bridge, inProgressRequests, resource]); + + const read = useCallback( + (id: number) => { + const rendererID = store.getRendererIDForElement(id); + if (rendererID != null) { + return resource.read({ id, rendererID }); + } else { + return null; + } + }, + [resource, store] + ); + + // "count" is intentionally passed so that it recreates the memoized object. + // eslint-disable-next-line react-hooks/exhaustive-deps + const value = useMemo(() => ({ read }), [count, read]); + + return ( + + {children} + + ); +} + +function hydrateHelper(dehydratedData: DehydratedData | null): Object | null { + if (dehydratedData !== null) { + return hydrate(dehydratedData.data, dehydratedData.cleaned); + } else { + return null; + } +} + +export { InspectedElementContext, InspectedElementContextController }; diff --git a/src/devtools/views/Components/SelectedElement.js b/src/devtools/views/Components/SelectedElement.js index 359e59b333..b8581983ab 100644 --- a/src/devtools/views/Components/SelectedElement.js +++ b/src/devtools/views/Components/SelectedElement.js @@ -7,6 +7,7 @@ import Button from '../Button'; import ButtonIcon from '../ButtonIcon'; import HooksTree from './HooksTree'; import InspectedElementTree from './InspectedElementTree'; +import { InspectedElementContext } from './InspectedElementContext'; import styles from './SelectedElement.css'; import { ElementTypeClass, @@ -25,13 +26,13 @@ export default function SelectedElement(_: Props) { const bridge = useContext(BridgeContext); const store = useContext(StoreContext); + const { read } = useContext(InspectedElementContext); + const element = selectedElementID !== null ? store.getElementByID(selectedElementID) : null; const inspectedElement = - selectedElementID != null - ? store.inspectedElementCache.read(selectedElementID) - : null; + selectedElementID != null ? read(selectedElementID) : null; const highlightElement = useCallback(() => { if (element !== null && selectedElementID !== null) { From 957c389566794307e663f53964ee181c7ce64df5 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Sat, 20 Apr 2019 10:35:19 -0700 Subject: [PATCH 03/18] Adding polling and initial stab at not serializing duplicate inspected Element props --- src/backend/renderer.js | 27 ++++ src/devtools/cache.js | 8 +- .../Components/InspectedElementContext.js | 116 +++++++++++------- .../views/Components/SelectedElement.js | 32 ++--- 4 files changed, 124 insertions(+), 59 deletions(-) diff --git a/src/backend/renderer.js b/src/backend/renderer.js index ce607379ce..f700bbf8be 100644 --- a/src/backend/renderer.js +++ b/src/backend/renderer.js @@ -959,6 +959,20 @@ export function attach( debug('updateFiberRecursively()', nextFiber, parentFiber); } const shouldIncludeInTree = !shouldFilterFiber(nextFiber); + + // If this is the most recently inspected Fiber, take note of whether it was part of the new commit. + // If not, we can avoid re-serializing its props and state if asked again. + // Note that we avoid even comparing IDs for fibers not in the tree, + // so that we don't inadvertantly add them to the ID Map. + if ( + shouldIncludeInTree && + inspectedElementID !== null && + inspectedElementID === getFiberID(getPrimaryFiber(nextFiber)) && + nextFiber.actualDuration > 0 + ) { + hasInspectedElementChanged = true; + } + const isSuspense = nextFiber.tag === SuspenseComponent; let shouldResetChildren = false; // The behavior of timed-out Suspense trees is unique. @@ -1632,16 +1646,29 @@ export function attach( }; } + let inspectedElementID: number | null = null; + let hasInspectedElementChanged: boolean = false; + function inspectElement(id: number): InspectedElement | null { + if (inspectedElementID === id && !hasInspectedElementChanged) { + // Optimization: Don't resend (and reserialize) unchanged props. + return null; + } + + inspectedElementID = id; + hasInspectedElementChanged = false; + let result = inspectElementRaw(id); if (result === null) { return null; } + // TODO Review sanitization approach for the below inspectable values. result.context = cleanForBridge(result.context); result.hooks = cleanForBridge(result.hooks); result.props = cleanForBridge(result.props); result.state = cleanForBridge(result.state); + return result; } diff --git a/src/devtools/cache.js b/src/devtools/cache.js index 11daf5d65f..ee3835fec3 100644 --- a/src/devtools/cache.js +++ b/src/devtools/cache.js @@ -157,7 +157,13 @@ export function createResource( write(key: Key, value: Value): void { const entriesForResource = ((entries.get(resource): any): Map); - entriesForResource.set(key, value); + + const resolvedResult: ResolvedResult = { + status: Resolved, + value, + }; + + entriesForResource.set(key, resolvedResult); }, }; diff --git a/src/devtools/views/Components/InspectedElementContext.js b/src/devtools/views/Components/InspectedElementContext.js index 53af90d5a1..196a98dcb4 100644 --- a/src/devtools/views/Components/InspectedElementContext.js +++ b/src/devtools/views/Components/InspectedElementContext.js @@ -2,7 +2,6 @@ import React, { createContext, - useCallback, useContext, useEffect, useMemo, @@ -11,6 +10,8 @@ import React, { import { createResource } from '../../cache'; import { BridgeContext, StoreContext } from '../context'; import { hydrate } from 'src/hydration'; +import { unstable_next as next } from 'scheduler'; +import { TreeContext } from './TreeContext'; import type { DehydratedData, @@ -18,20 +19,10 @@ import type { } from 'src/devtools/views/Components/types'; import type { Resource } from '../../cache'; -// TODO Something needs to poll for (unprompted) updates. - -// TODO The curretn approach caches resources permanently. -// We won't even ask for an update if an element is reselected. -// I think we need to separate the polling for an update from the suspense cache. -// This way we can always resened (and poll on an interval) for the selected id, -// and the cache here can just invalidate itself as responses stream in. - -type Params = {| - id: number, - rendererID: number, -|}; +// TODO This isn't using the "two setState" pattern and updates sometimes feel janky. type Context = {| + inspectedElementID: number | null, read(id: number): InspectedElement | null, |}; @@ -52,17 +43,57 @@ function InspectedElementContextController({ children }: Props) { const bridge = useContext(BridgeContext); const store = useContext(StoreContext); - const [count, setCount] = useState(0); + const { selectedElementID } = useContext(TreeContext); + const [inspectedElement, setInspectedElement] = useState<{ + id: number | null, + inspectedElement: InspectedElement | null, + }>({ + id: selectedElementID, + inspectedElement: null, + }); + if (inspectedElement.id !== selectedElementID) { + if (selectedElementID === null) { + setInspectedElement({ + id: selectedElementID, + inspectedElement: null, + }); + } else { + next(() => + setInspectedElement({ + id: selectedElementID, + inspectedElement: null, + }) + ); + } + } + + useEffect(() => { + if (inspectedElement.id === null) { + return () => {}; + } + + const rendererID = store.getRendererIDForElement(inspectedElement.id); + + const requestUpdate = () => { + bridge.send('inspectElement', { id: inspectedElement.id, rendererID }); + }; + + requestUpdate(); + + const intervalID = setInterval(requestUpdate, 1000); + + return () => clearInterval(intervalID); + }, [bridge, inspectedElement.id, store]); const inProgressRequests = useMemo>( () => new Map(), [] ); - const resource = useMemo>( + const resource = useMemo>( () => createResource( - ({ id, rendererID }: Params) => { + (id: number) => { let request = inProgressRequests.get(id); if (request != null) { return request.promise; @@ -71,28 +102,31 @@ function InspectedElementContextController({ children }: Props) { let resolveFn = ((null: any): ResolveFn); const promise = new Promise(resolve => { resolveFn = resolve; - - bridge.send('inspectElement', { id, rendererID }); }); inProgressRequests.set(id, { promise, resolveFn }); return promise; }, - ({ id, rendererID }: Params) => id + (id: number) => id ), - [bridge, inProgressRequests] + [inProgressRequests] ); useEffect(() => { - const onInspectedElement = (inspectedElement: InspectedElement | null) => { - if (inspectedElement != null) { - const id = inspectedElement.id; + const onInspectedElement = ( + inspectedElementRaw: InspectedElement | null + ) => { + if (inspectedElementRaw != null) { + const id = inspectedElementRaw.id; - inspectedElement.context = hydrateHelper(inspectedElement.context); - inspectedElement.hooks = hydrateHelper(inspectedElement.hooks); - inspectedElement.props = hydrateHelper(inspectedElement.props); - inspectedElement.state = hydrateHelper(inspectedElement.state); + const inspectedElement = (({ + ...inspectedElementRaw, + context: hydrateHelper(inspectedElementRaw.context), + hooks: hydrateHelper(inspectedElementRaw.hooks), + props: hydrateHelper(inspectedElementRaw.props), + state: hydrateHelper(inspectedElementRaw.state), + }: any): InspectedElement); const request = inProgressRequests.get(id); if (request != null) { @@ -101,8 +135,10 @@ function InspectedElementContextController({ children }: Props) { } else { resource.write(id, inspectedElement); - // Schedule update with React. - setCount(count => count + 1); + // Schedule update with React if necessary. + setInspectedElement(prevState => + prevState.id === id ? { id, inspectedElement } : prevState + ); } } }; @@ -111,22 +147,16 @@ function InspectedElementContextController({ children }: Props) { return () => bridge.removeListener('inspectElement', onInspectedElement); }, [bridge, inProgressRequests, resource]); - const read = useCallback( - (id: number) => { - const rendererID = store.getRendererIDForElement(id); - if (rendererID != null) { - return resource.read({ id, rendererID }); - } else { - return null; - } - }, - [resource, store] + // We intentionally use the broader inspectedElement object, rather than the id, + // to enable updates to be scheduled with React after the cache has been invalidated. + const value = useMemo( + () => ({ + inspectedElementID: inspectedElement.id, + read: resource.read, + }), + [inspectedElement, resource.read] ); - // "count" is intentionally passed so that it recreates the memoized object. - // eslint-disable-next-line react-hooks/exhaustive-deps - const value = useMemo(() => ({ read }), [count, read]); - return ( {children} diff --git a/src/devtools/views/Components/SelectedElement.js b/src/devtools/views/Components/SelectedElement.js index b8581983ab..e0aee48cdf 100644 --- a/src/devtools/views/Components/SelectedElement.js +++ b/src/devtools/views/Components/SelectedElement.js @@ -22,51 +22,53 @@ import type { Element, InspectedElement } from './types'; export type Props = {||}; export default function SelectedElement(_: Props) { - const { selectedElementID, viewElementSource } = useContext(TreeContext); + const { viewElementSource } = useContext(TreeContext); const bridge = useContext(BridgeContext); const store = useContext(StoreContext); - const { read } = useContext(InspectedElementContext); + const { inspectedElementID, read } = useContext(InspectedElementContext); const element = - selectedElementID !== null ? store.getElementByID(selectedElementID) : null; + inspectedElementID !== null + ? store.getElementByID(inspectedElementID) + : null; const inspectedElement = - selectedElementID != null ? read(selectedElementID) : null; + inspectedElementID != null ? read(inspectedElementID) : null; const highlightElement = useCallback(() => { - if (element !== null && selectedElementID !== null) { - const rendererID = store.getRendererIDForElement(selectedElementID); + if (element !== null && inspectedElementID !== null) { + const rendererID = store.getRendererIDForElement(inspectedElementID); if (rendererID !== null) { bridge.send('highlightElementInDOM', { displayName: element.displayName, hideAfterTimeout: true, - id: selectedElementID, + id: inspectedElementID, openNativeElementsPanel: true, rendererID, scrollIntoView: true, }); } } - }, [bridge, element, selectedElementID, store]); + }, [bridge, element, inspectedElementID, store]); const logElement = useCallback(() => { - if (selectedElementID !== null) { - const rendererID = store.getRendererIDForElement(selectedElementID); + if (inspectedElementID !== null) { + const rendererID = store.getRendererIDForElement(inspectedElementID); if (rendererID !== null) { bridge.send('logElementToConsole', { - id: selectedElementID, + id: inspectedElementID, rendererID, }); } } - }, [bridge, selectedElementID, store]); + }, [bridge, inspectedElementID, store]); const viewSource = useCallback(() => { - if (viewElementSource != null && selectedElementID !== null) { - viewElementSource(selectedElementID); + if (viewElementSource != null && inspectedElementID !== null) { + viewElementSource(inspectedElementID); } - }, [selectedElementID, viewElementSource]); + }, [inspectedElementID, viewElementSource]); if (element === null) { return ( From 3de18de25ec3c8b28423ab53a760f4cc2087c95c Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Sun, 21 Apr 2019 12:16:42 -0700 Subject: [PATCH 04/18] Tried to implement two setState pattern, but it does not feel right --- .../Components/InspectedElementContext.js | 48 ++------ .../views/Components/SelectedElement.js | 4 +- src/devtools/views/Components/TreeContext.js | 104 ++++++++++++------ 3 files changed, 84 insertions(+), 72 deletions(-) diff --git a/src/devtools/views/Components/InspectedElementContext.js b/src/devtools/views/Components/InspectedElementContext.js index 196a98dcb4..e3ea5fd246 100644 --- a/src/devtools/views/Components/InspectedElementContext.js +++ b/src/devtools/views/Components/InspectedElementContext.js @@ -10,7 +10,6 @@ import React, { import { createResource } from '../../cache'; import { BridgeContext, StoreContext } from '../context'; import { hydrate } from 'src/hydration'; -import { unstable_next as next } from 'scheduler'; import { TreeContext } from './TreeContext'; import type { @@ -19,10 +18,7 @@ import type { } from 'src/devtools/views/Components/types'; import type { Resource } from '../../cache'; -// TODO This isn't using the "two setState" pattern and updates sometimes feel janky. - type Context = {| - inspectedElementID: number | null, read(id: number): InspectedElement | null, |}; @@ -42,40 +38,19 @@ type Props = {| function InspectedElementContextController({ children }: Props) { const bridge = useContext(BridgeContext); const store = useContext(StoreContext); + const { inspectedElementID } = useContext(TreeContext); - const { selectedElementID } = useContext(TreeContext); - const [inspectedElement, setInspectedElement] = useState<{ - id: number | null, - inspectedElement: InspectedElement | null, - }>({ - id: selectedElementID, - inspectedElement: null, - }); - if (inspectedElement.id !== selectedElementID) { - if (selectedElementID === null) { - setInspectedElement({ - id: selectedElementID, - inspectedElement: null, - }); - } else { - next(() => - setInspectedElement({ - id: selectedElementID, - inspectedElement: null, - }) - ); - } - } + const [count, setCount] = useState(0); useEffect(() => { - if (inspectedElement.id === null) { + if (inspectedElementID === null) { return () => {}; } - const rendererID = store.getRendererIDForElement(inspectedElement.id); + const rendererID = store.getRendererIDForElement(inspectedElementID); const requestUpdate = () => { - bridge.send('inspectElement', { id: inspectedElement.id, rendererID }); + bridge.send('inspectElement', { id: inspectedElementID, rendererID }); }; requestUpdate(); @@ -83,7 +58,7 @@ function InspectedElementContextController({ children }: Props) { const intervalID = setInterval(requestUpdate, 1000); return () => clearInterval(intervalID); - }, [bridge, inspectedElement.id, store]); + }, [bridge, inspectedElementID, store]); const inProgressRequests = useMemo>( () => new Map(), @@ -136,9 +111,7 @@ function InspectedElementContextController({ children }: Props) { resource.write(id, inspectedElement); // Schedule update with React if necessary. - setInspectedElement(prevState => - prevState.id === id ? { id, inspectedElement } : prevState - ); + setCount(count => count + 1); } } }; @@ -147,14 +120,13 @@ function InspectedElementContextController({ children }: Props) { return () => bridge.removeListener('inspectElement', onInspectedElement); }, [bridge, inProgressRequests, resource]); - // We intentionally use the broader inspectedElement object, rather than the id, - // to enable updates to be scheduled with React after the cache has been invalidated. const value = useMemo( () => ({ - inspectedElementID: inspectedElement.id, read: resource.read, }), - [inspectedElement, resource.read] + // Count is used to invalidate the cache and schedule an update with React. + // eslint-disable-next-line react-hooks/exhaustive-deps + [count, resource.read] ); return ( diff --git a/src/devtools/views/Components/SelectedElement.js b/src/devtools/views/Components/SelectedElement.js index e0aee48cdf..c3de9a2718 100644 --- a/src/devtools/views/Components/SelectedElement.js +++ b/src/devtools/views/Components/SelectedElement.js @@ -22,11 +22,11 @@ import type { Element, InspectedElement } from './types'; export type Props = {||}; export default function SelectedElement(_: Props) { - const { viewElementSource } = useContext(TreeContext); + const { inspectedElementID, viewElementSource } = useContext(TreeContext); const bridge = useContext(BridgeContext); const store = useContext(StoreContext); - const { inspectedElementID, read } = useContext(InspectedElementContext); + const { read } = useContext(InspectedElementContext); const element = inspectedElementID !== null diff --git a/src/devtools/views/Components/TreeContext.js b/src/devtools/views/Components/TreeContext.js index 5c8a300e8a..ab1d7031cf 100644 --- a/src/devtools/views/Components/TreeContext.js +++ b/src/devtools/views/Components/TreeContext.js @@ -27,16 +27,13 @@ import React, { useReducer, useRef, } from 'react'; +import { unstable_next as next } from 'scheduler'; import { createRegExp } from '../utils'; import { BridgeContext, StoreContext } from '../context'; import Store from '../../store'; import type { Element } from './types'; -// TODO Use two setState pattern for selecting Fibers: -// The first update should be default priority and should select a new element in the Tree. -// The second update should be deferred priority and should trigger suspense. - type Context = {| // Tree baseDepth: number, @@ -67,6 +64,10 @@ type Context = {| // Injected by parent HTML/JavaScript viewElementSource: Function | null, + + // Inspection element panel + // Updated separately so we can avoid suspending when selection changes + inspectedElementID: number | null, |}; const TreeContext = createContext(((null: any): Context)); @@ -88,6 +89,9 @@ type State = {| ownerStack: Array, ownerStackIndex: number | null, _ownerFlatTree: Array | null, + + // Inspection element panel + inspectedElementID: number | null, |}; type Action = {| @@ -103,7 +107,8 @@ type Action = {| | 'SELECT_PARENT_ELEMENT_IN_TREE' | 'SELECT_PREVIOUS_ELEMENT_IN_TREE' | 'SELECT_OWNER' - | 'SET_SEARCH_TEXT', + | 'SET_SEARCH_TEXT' + | 'UPDATE_INSPECTED_ELEMENT_ID', payload?: any, |}; @@ -566,6 +571,24 @@ function reduceOwnersState(store: Store, state: State, action: Action): State { }; } +function reduceSuspenseState( + store: Store, + state: State, + action: Action +): State { + const { type } = action; + switch (type) { + case 'UPDATE_INSPECTED_ELEMENT_ID': + return { + ...state, + inspectedElementID: state.selectedElementID, + }; + default: + // React can bailout of no-op updates. + return state; + } +} + type Props = {| children: React$Node, viewElementSource: Function | null, @@ -596,10 +619,12 @@ function TreeContextController({ children, viewElementSource }: Props) { case 'SELECT_PARENT_ELEMENT_IN_TREE': case 'SELECT_PREVIOUS_ELEMENT_IN_TREE': case 'SELECT_OWNER': + case 'UPDATE_INSPECTED_ELEMENT_ID': case 'SET_SEARCH_TEXT': state = reduceTreeState(store, state, action); state = reduceSearchState(store, state, action); state = reduceOwnersState(store, state, action); + state = reduceSuspenseState(store, state, action); // If the selected ID is in a collapsed subtree, reset the selected index to null. // We'll know the correct index after the layout effect will toggle the tree, @@ -638,8 +663,19 @@ function TreeContextController({ children, viewElementSource }: Props) { ownerStack: [], ownerStackIndex: null, _ownerFlatTree: null, + + // Inspection element panel + inspectedElementID: null, }); + const dispatchWrapper = useCallback( + params => { + dispatch(params); + next(() => dispatch({ type: 'UPDATE_INSPECTED_ELEMENT_ID' })); + }, + [dispatch] + ); + const getElementAtIndex = useCallback( (index: number) => { return state._ownerFlatTree === null @@ -650,49 +686,50 @@ function TreeContextController({ children, viewElementSource }: Props) { ); const selectElementAtIndex = useCallback( (index: number) => - dispatch({ type: 'SELECT_ELEMENT_AT_INDEX', payload: index }), - [dispatch] + dispatchWrapper({ type: 'SELECT_ELEMENT_AT_INDEX', payload: index }), + [dispatchWrapper] ); const selectElementByID = useCallback( (id: number | null) => - dispatch({ type: 'SELECT_ELEMENT_BY_ID', payload: id }), - [dispatch] + dispatchWrapper({ type: 'SELECT_ELEMENT_BY_ID', payload: id }), + [dispatchWrapper] ); const setSearchText = useCallback( - (text: string) => dispatch({ type: 'SET_SEARCH_TEXT', payload: text }), - [dispatch] + (text: string) => + dispatchWrapper({ type: 'SET_SEARCH_TEXT', payload: text }), + [dispatchWrapper] ); const goToNextSearchResult = useCallback( - () => dispatch({ type: 'GO_TO_NEXT_SEARCH_RESULT' }), - [dispatch] + () => dispatchWrapper({ type: 'GO_TO_NEXT_SEARCH_RESULT' }), + [dispatchWrapper] ); const goToPreviousSearchResult = useCallback( - () => dispatch({ type: 'GO_TO_PREVIOUS_SEARCH_RESULT' }), - [dispatch] + () => dispatchWrapper({ type: 'GO_TO_PREVIOUS_SEARCH_RESULT' }), + [dispatchWrapper] ); const resetOwnerStack = useCallback( - () => dispatch({ type: 'RESET_OWNER_STACK' }), - [dispatch] + () => dispatchWrapper({ type: 'RESET_OWNER_STACK' }), + [dispatchWrapper] ); const selectChildElementInTree = useCallback( - () => dispatch({ type: 'SELECT_CHILD_ELEMENT_IN_TREE' }), - [dispatch] + () => dispatchWrapper({ type: 'SELECT_CHILD_ELEMENT_IN_TREE' }), + [dispatchWrapper] ); const selectNextElementInTree = useCallback( - () => dispatch({ type: 'SELECT_NEXT_ELEMENT_IN_TREE' }), - [dispatch] + () => dispatchWrapper({ type: 'SELECT_NEXT_ELEMENT_IN_TREE' }), + [dispatchWrapper] ); const selectParentElementInTree = useCallback( - () => dispatch({ type: 'SELECT_PARENT_ELEMENT_IN_TREE' }), - [dispatch] + () => dispatchWrapper({ type: 'SELECT_PARENT_ELEMENT_IN_TREE' }), + [dispatchWrapper] ); const selectPreviousElementInTree = useCallback( - () => dispatch({ type: 'SELECT_PREVIOUS_ELEMENT_IN_TREE' }), - [dispatch] + () => dispatchWrapper({ type: 'SELECT_PREVIOUS_ELEMENT_IN_TREE' }), + [dispatchWrapper] ); const selectOwner = useCallback( - (id: number) => dispatch({ type: 'SELECT_OWNER', payload: id }), - [dispatch] + (id: number) => dispatchWrapper({ type: 'SELECT_OWNER', payload: id }), + [dispatchWrapper] ); const value = useMemo( @@ -724,6 +761,9 @@ function TreeContextController({ children, viewElementSource }: Props) { resetOwnerStack, selectOwner, + // Inspection element panel + inspectedElementID: state.inspectedElementID, + // Injected by parent HTML/JavaScript viewElementSource, }), @@ -748,10 +788,10 @@ function TreeContextController({ children, viewElementSource }: Props) { // Listen for host element selections. useEffect(() => { const handleSelectFiber = (id: number) => - dispatch({ type: 'SELECT_ELEMENT_BY_ID', payload: id }); + dispatchWrapper({ type: 'SELECT_ELEMENT_BY_ID', payload: id }); bridge.addListener('selectFiber', handleSelectFiber); return () => bridge.removeListener('selectFiber', handleSelectFiber); - }, [bridge, dispatch]); + }, [bridge, dispatchWrapper]); // If a newly-selected search result or inspection selection is inside of a collapsed subtree, auto expand it. // This needs to be a layout effect to avoid temporarily flashing an incorrect selection. @@ -775,7 +815,7 @@ function TreeContextController({ children, viewElementSource }: Props) { addedElementIDs, removedElementIDs, ]: Array) => { - dispatch({ + dispatchWrapper({ type: 'HANDLE_STORE_MUTATION', payload: [addedElementIDs, removedElementIDs], }); @@ -786,7 +826,7 @@ function TreeContextController({ children, viewElementSource }: Props) { // At the moment, we can treat this as a mutation. // We don't know which Elements were newly added/removed, but that should be okay in this case. // It would only impact the search state, which is unlikely to exist yet at this point. - dispatch({ + dispatchWrapper({ type: 'HANDLE_STORE_MUTATION', payload: [new Uint32Array(0), new Uint32Array(0)], }); @@ -795,7 +835,7 @@ function TreeContextController({ children, viewElementSource }: Props) { store.addListener('mutated', handleStoreMutated); return () => store.removeListener('mutated', handleStoreMutated); - }, [dispatch, initialRevision, store]); + }, [dispatchWrapper, initialRevision, store]); return {children}; } From 5970bf4b40335086d53403b59d2916bca6e7fc1c Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 22 Apr 2019 08:52:15 -0700 Subject: [PATCH 05/18] Changed polling approach. Fixed remove event typo. --- .../Components/InspectedElementContext.js | 34 ++++++++++++++++--- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/src/devtools/views/Components/InspectedElementContext.js b/src/devtools/views/Components/InspectedElementContext.js index e3ea5fd246..31c7f6431c 100644 --- a/src/devtools/views/Components/InspectedElementContext.js +++ b/src/devtools/views/Components/InspectedElementContext.js @@ -42,6 +42,7 @@ function InspectedElementContextController({ children }: Props) { const [count, setCount] = useState(0); + // This effect handler polls for updates on the currently selected element. useEffect(() => { if (inspectedElementID === null) { return () => {}; @@ -49,15 +50,37 @@ function InspectedElementContextController({ children }: Props) { const rendererID = store.getRendererIDForElement(inspectedElementID); - const requestUpdate = () => { + let timeoutID: TimeoutID | null = null; + + const sendRequest = () => { + timeoutID = null; + bridge.send('inspectElement', { id: inspectedElementID, rendererID }); }; - requestUpdate(); + // Send the initial inspection request. + // We'll poll for an update in the response handler below. + sendRequest(); - const intervalID = setInterval(requestUpdate, 1000); + const onInspectedElement = (inspectedElement: InspectedElement | null) => { + if ( + inspectedElement !== null && + inspectedElement.id === inspectedElementID + ) { + // If this is the element we requested, wait a little bit and then ask for an update. + timeoutID = setTimeout(sendRequest, 1000); + } + }; - return () => clearInterval(intervalID); + bridge.addListener('inspectedElement', onInspectedElement); + + return () => { + bridge.removeListener('inspectedElement', onInspectedElement); + + if (timeoutID !== null) { + clearTimeout(timeoutID); + } + }; }, [bridge, inspectedElementID, store]); const inProgressRequests = useMemo>( @@ -88,6 +111,7 @@ function InspectedElementContextController({ children }: Props) { [inProgressRequests] ); + // This effect handler invalidates the suspense cache and schedules rendering updates with React. useEffect(() => { const onInspectedElement = ( inspectedElementRaw: InspectedElement | null @@ -117,7 +141,7 @@ function InspectedElementContextController({ children }: Props) { }; bridge.addListener('inspectedElement', onInspectedElement); - return () => bridge.removeListener('inspectElement', onInspectedElement); + return () => bridge.removeListener('inspectedElement', onInspectedElement); }, [bridge, inProgressRequests, resource]); const value = useMemo( From 70be637d48d78e932809b96a2b49a572e03e8e54 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 22 Apr 2019 09:03:25 -0700 Subject: [PATCH 06/18] Don't schedule an update with React unless the curent element was invalidated --- .../Components/InspectedElementContext.js | 90 ++++++++++--------- 1 file changed, 46 insertions(+), 44 deletions(-) diff --git a/src/devtools/views/Components/InspectedElementContext.js b/src/devtools/views/Components/InspectedElementContext.js index 31c7f6431c..21c5aefb5f 100644 --- a/src/devtools/views/Components/InspectedElementContext.js +++ b/src/devtools/views/Components/InspectedElementContext.js @@ -42,47 +42,6 @@ function InspectedElementContextController({ children }: Props) { const [count, setCount] = useState(0); - // This effect handler polls for updates on the currently selected element. - useEffect(() => { - if (inspectedElementID === null) { - return () => {}; - } - - const rendererID = store.getRendererIDForElement(inspectedElementID); - - let timeoutID: TimeoutID | null = null; - - const sendRequest = () => { - timeoutID = null; - - bridge.send('inspectElement', { id: inspectedElementID, rendererID }); - }; - - // Send the initial inspection request. - // We'll poll for an update in the response handler below. - sendRequest(); - - const onInspectedElement = (inspectedElement: InspectedElement | null) => { - if ( - inspectedElement !== null && - inspectedElement.id === inspectedElementID - ) { - // If this is the element we requested, wait a little bit and then ask for an update. - timeoutID = setTimeout(sendRequest, 1000); - } - }; - - bridge.addListener('inspectedElement', onInspectedElement); - - return () => { - bridge.removeListener('inspectedElement', onInspectedElement); - - if (timeoutID !== null) { - clearTimeout(timeoutID); - } - }; - }, [bridge, inspectedElementID, store]); - const inProgressRequests = useMemo>( () => new Map(), [] @@ -134,15 +93,58 @@ function InspectedElementContextController({ children }: Props) { } else { resource.write(id, inspectedElement); - // Schedule update with React if necessary. - setCount(count => count + 1); + // Schedule update with React if the curently-selected element has been invalidated. + if (id === inspectedElementID) { + setCount(count => count + 1); + } } } }; bridge.addListener('inspectedElement', onInspectedElement); return () => bridge.removeListener('inspectedElement', onInspectedElement); - }, [bridge, inProgressRequests, resource]); + }, [bridge, inProgressRequests, inspectedElementID, resource]); + + // This effect handler polls for updates on the currently selected element. + useEffect(() => { + if (inspectedElementID === null) { + return () => {}; + } + + const rendererID = store.getRendererIDForElement(inspectedElementID); + + let timeoutID: TimeoutID | null = null; + + const sendRequest = () => { + timeoutID = null; + + bridge.send('inspectElement', { id: inspectedElementID, rendererID }); + }; + + // Send the initial inspection request. + // We'll poll for an update in the response handler below. + sendRequest(); + + const onInspectedElement = (inspectedElement: InspectedElement | null) => { + if ( + inspectedElement !== null && + inspectedElement.id === inspectedElementID + ) { + // If this is the element we requested, wait a little bit and then ask for an update. + timeoutID = setTimeout(sendRequest, 1000); + } + }; + + bridge.addListener('inspectedElement', onInspectedElement); + + return () => { + bridge.removeListener('inspectedElement', onInspectedElement); + + if (timeoutID !== null) { + clearTimeout(timeoutID); + } + }; + }, [bridge, inspectedElementID, store]); const value = useMemo( () => ({ From d53ae2ea8a5db88232c6802fc8f480cb13c23576 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 22 Apr 2019 10:19:59 -0700 Subject: [PATCH 07/18] Refactored TreeContext to use less memoization (based on feedback from Sebastian) --- src/devtools/views/Components/Element.js | 34 +- .../Components/InspectedElementContext.js | 4 +- src/devtools/views/Components/OwnersStack.js | 20 +- src/devtools/views/Components/SearchInput.js | 40 ++- .../views/Components/SelectedElement.js | 22 +- src/devtools/views/Components/Tree.js | 47 +-- src/devtools/views/Components/TreeContext.js | 314 +++++++----------- .../Components/ViewElementSourceContext.js | 8 + src/devtools/views/DevTools.js | 85 ++--- .../views/Profiler/ProfilerContext.js | 15 +- 10 files changed, 268 insertions(+), 321 deletions(-) create mode 100644 src/devtools/views/Components/ViewElementSourceContext.js diff --git a/src/devtools/views/Components/Element.js b/src/devtools/views/Components/Element.js index 5e5b52fe08..0fa93b40ac 100644 --- a/src/devtools/views/Components/Element.js +++ b/src/devtools/views/Components/Element.js @@ -13,7 +13,7 @@ import { ElementTypeClass, ElementTypeFunction } from 'src/devtools/types'; import Store from 'src/devtools/store'; import ButtonIcon from '../ButtonIcon'; import { createRegExp } from '../utils'; -import { TreeContext } from './TreeContext'; +import { TreeDispatcherContext, TreeStateContext } from './TreeContext'; import { StoreContext } from '../context'; import type { ItemData } from './Tree'; @@ -28,18 +28,21 @@ type Props = { }; export default function ElementView({ data, index, style }: Props) { - const [isHovered, setIsHovered] = useState(false); + const store = useContext(StoreContext); const { baseDepth, - getElementAtIndex, + ownerFlatTree, ownerStack, - selectOwner, selectedElementID, - selectElementByID, - } = useContext(TreeContext); - const store = useContext(StoreContext); + } = useContext(TreeStateContext); + const dispatch = useContext(TreeDispatcherContext); - const element = getElementAtIndex(index); + const element = + ownerFlatTree !== null + ? store.getElementByID(ownerFlatTree[index]) + : store.getElementAtIndex(index); + + const [isHovered, setIsHovered] = useState(false); const { lastScrolledIDRef, @@ -52,9 +55,9 @@ export default function ElementView({ data, index, style }: Props) { const handleDoubleClick = useCallback(() => { if (id !== null) { - selectOwner(id); + dispatch({ type: 'SELECT_OWNER', payload: id }); } - }, [id, selectOwner]); + }, [dispatch, id]); const scrollAnchorStartRef = useRef(null); const scrollAnchorEndRef = useRef(null); @@ -102,10 +105,13 @@ export default function ElementView({ data, index, style }: Props) { const handleMouseDown = useCallback( ({ metaKey }) => { if (id !== null) { - selectElementByID(metaKey ? null : id); + dispatch({ + type: 'SELECT_ELEMENT_BY_ID', + payload: metaKey ? null : id, + }); } }, - [id, selectElementByID] + [dispatch, id] ); const handleMouseEnter = useCallback(() => { @@ -234,7 +240,9 @@ type DisplayNameProps = {| |}; function DisplayName({ displayName, id }: DisplayNameProps) { - const { searchIndex, searchResults, searchText } = useContext(TreeContext); + const { searchIndex, searchResults, searchText } = useContext( + TreeStateContext + ); const isSearchResult = useMemo(() => { return searchResults.includes(id); }, [id, searchResults]); diff --git a/src/devtools/views/Components/InspectedElementContext.js b/src/devtools/views/Components/InspectedElementContext.js index 21c5aefb5f..fbbc572f7b 100644 --- a/src/devtools/views/Components/InspectedElementContext.js +++ b/src/devtools/views/Components/InspectedElementContext.js @@ -10,7 +10,7 @@ import React, { import { createResource } from '../../cache'; import { BridgeContext, StoreContext } from '../context'; import { hydrate } from 'src/hydration'; -import { TreeContext } from './TreeContext'; +import { TreeStateContext } from './TreeContext'; import type { DehydratedData, @@ -38,7 +38,7 @@ type Props = {| function InspectedElementContextController({ children }: Props) { const bridge = useContext(BridgeContext); const store = useContext(StoreContext); - const { inspectedElementID } = useContext(TreeContext); + const { inspectedElementID } = useContext(TreeStateContext); const [count, setCount] = useState(0); diff --git a/src/devtools/views/Components/OwnersStack.js b/src/devtools/views/Components/OwnersStack.js index 860514fc78..b2f0c3299d 100644 --- a/src/devtools/views/Components/OwnersStack.js +++ b/src/devtools/views/Components/OwnersStack.js @@ -11,7 +11,7 @@ import { Menu, MenuList, MenuButton, MenuItem } from '@reach/menu-button'; import Button from '../Button'; import ButtonIcon from '../ButtonIcon'; import Toggle from '../Toggle'; -import { TreeContext } from './TreeContext'; +import { TreeDispatcherContext, TreeStateContext } from './TreeContext'; import { StoreContext } from '../context'; import { useIsOverflowing } from '../hooks'; @@ -20,9 +20,8 @@ import type { Element } from './types'; import styles from './OwnersStack.css'; export default function OwnerStack() { - const { ownerStack, ownerStackIndex, resetOwnerStack } = useContext( - TreeContext - ); + const { ownerStack, ownerStackIndex } = useContext(TreeStateContext); + const dispatch = useContext(TreeDispatcherContext); const [elementsTotalWidth, setElementsTotalWidth] = useState(0); const elementsBarRef = useRef(null); @@ -73,7 +72,7 @@ export default function OwnerStack() {