diff --git a/src/__tests__/__snapshots__/inspectedElementContext-test.js.snap b/src/__tests__/__snapshots__/inspectedElementContext-test.js.snap index 421b1011dc..db663bc853 100644 --- a/src/__tests__/__snapshots__/inspectedElementContext-test.js.snap +++ b/src/__tests__/__snapshots__/inspectedElementContext-test.js.snap @@ -369,6 +369,7 @@ exports[`InspectedElementContext should not tear if hydration is requested after "hooks": null, "props": { "nestedObject": { + "value": 1, "a": {} } }, @@ -385,6 +386,7 @@ exports[`InspectedElementContext should not tear if hydration is requested after "hooks": null, "props": { "nestedObject": { + "value": 2, "a": { "value": 2, "b": { diff --git a/src/__tests__/inspectedElementContext-test.js b/src/__tests__/inspectedElementContext-test.js index 6c0197371f..105a23135f 100644 --- a/src/__tests__/inspectedElementContext-test.js +++ b/src/__tests__/inspectedElementContext-test.js @@ -1,7 +1,7 @@ // @flow import typeof ReactTestRenderer from 'react-test-renderer'; -import type { GetPath } from 'src/devtools/views/Components/InspectedElementContext'; +import type { GetInspectedElementPath } from 'src/devtools/views/Components/InspectedElementContext'; import type Bridge from 'src/bridge'; import type Store from 'src/devtools/store'; @@ -81,8 +81,8 @@ describe('InspectedElementContext', () => { let didFinish = false; function Suspender({ target }) { - const { read } = React.useContext(InspectedElementContext); - const inspectedElement = read(id); + const { getInspectedElement } = React.useContext(InspectedElementContext); + const inspectedElement = getInspectedElement(id); expect(inspectedElement).toMatchSnapshot(`1: Inspected element ${id}`); didFinish = true; return null; @@ -121,8 +121,8 @@ describe('InspectedElementContext', () => { let inspectedElement = null; function Suspender({ target }) { - const { read } = React.useContext(InspectedElementContext); - inspectedElement = read(id); + const { getInspectedElement } = React.useContext(InspectedElementContext); + inspectedElement = getInspectedElement(id); return null; } @@ -189,8 +189,8 @@ describe('InspectedElementContext', () => { let inspectedElement = null; function Suspender({ target }) { - const { read } = React.useContext(InspectedElementContext); - inspectedElement = read(target); + const { getInspectedElement } = React.useContext(InspectedElementContext); + inspectedElement = getInspectedElement(target); return null; } @@ -283,8 +283,8 @@ describe('InspectedElementContext', () => { let inspectedElement = null; function Suspender({ target }) { - const { read } = React.useContext(InspectedElementContext); - inspectedElement = read(id); + const { getInspectedElement } = React.useContext(InspectedElementContext); + inspectedElement = getInspectedElement(id); return null; } @@ -371,8 +371,8 @@ describe('InspectedElementContext', () => { let didFinish = false; function Suspender({ target }) { - const { read } = React.useContext(InspectedElementContext); - const inspectedElement = read(id); + const { getInspectedElement } = React.useContext(InspectedElementContext); + const inspectedElement = getInspectedElement(id); expect(inspectedElement).toMatchSnapshot(`1: Inspected element ${id}`); didFinish = true; return null; @@ -434,13 +434,13 @@ describe('InspectedElementContext', () => { const id = ((store.getElementIDAtIndex(0): any): number); - let getPath: GetPath = ((null: any): GetPath); + let getInspectedElementPath: GetInspectedElementPath = ((null: any): GetInspectedElementPath); let inspectedElement = null; function Suspender({ target }) { const context = React.useContext(InspectedElementContext); - getPath = context.getPath; - inspectedElement = context.read(target); + getInspectedElementPath = context.getInspectedElementPath; + inspectedElement = context.getInspectedElement(target); return null; } @@ -458,13 +458,13 @@ describe('InspectedElementContext', () => { ), false ); - expect(getPath).not.toBeNull(); + expect(getInspectedElementPath).not.toBeNull(); expect(inspectedElement).not.toBeNull(); expect(inspectedElement).toMatchSnapshot('1: Initially inspect element'); inspectedElement = null; TestUtils.act(() => { - getPath(id, ['props', 'nestedObject', 'a']); + getInspectedElementPath(id, ['props', 'nestedObject', 'a']); jest.runOnlyPendingTimers(); }); expect(inspectedElement).not.toBeNull(); @@ -472,7 +472,7 @@ describe('InspectedElementContext', () => { inspectedElement = null; TestUtils.act(() => { - getPath(id, ['props', 'nestedObject', 'a', 'b', 'c']); + getInspectedElementPath(id, ['props', 'nestedObject', 'a', 'b', 'c']); jest.runOnlyPendingTimers(); }); expect(inspectedElement).not.toBeNull(); @@ -482,7 +482,15 @@ describe('InspectedElementContext', () => { inspectedElement = null; TestUtils.act(() => { - getPath(id, ['props', 'nestedObject', 'a', 'b', 'c', 0, 'd']); + getInspectedElementPath(id, [ + 'props', + 'nestedObject', + 'a', + 'b', + 'c', + 0, + 'd', + ]); jest.runOnlyPendingTimers(); }); expect(inspectedElement).not.toBeNull(); @@ -492,7 +500,7 @@ describe('InspectedElementContext', () => { inspectedElement = null; TestUtils.act(() => { - getPath(id, ['hooks', 0, 'value']); + getInspectedElementPath(id, ['hooks', 0, 'value']); jest.runOnlyPendingTimers(); }); expect(inspectedElement).not.toBeNull(); @@ -500,7 +508,7 @@ describe('InspectedElementContext', () => { inspectedElement = null; TestUtils.act(() => { - getPath(id, ['hooks', 0, 'value', 'foo', 'bar']); + getInspectedElementPath(id, ['hooks', 0, 'value', 'foo', 'bar']); jest.runOnlyPendingTimers(); }); expect(inspectedElement).not.toBeNull(); @@ -542,13 +550,13 @@ describe('InspectedElementContext', () => { const id = ((store.getElementIDAtIndex(0): any): number); - let getPath: GetPath = ((null: any): GetPath); + let getInspectedElementPath: GetInspectedElementPath = ((null: any): GetInspectedElementPath); let inspectedElement = null; function Suspender({ target }) { const context = React.useContext(InspectedElementContext); - getPath = context.getPath; - inspectedElement = context.read(id); + getInspectedElementPath = context.getInspectedElementPath; + inspectedElement = context.getInspectedElement(id); return null; } @@ -566,13 +574,13 @@ describe('InspectedElementContext', () => { ), false ); - expect(getPath).not.toBeNull(); + expect(getInspectedElementPath).not.toBeNull(); expect(inspectedElement).not.toBeNull(); expect(inspectedElement).toMatchSnapshot('1: Initially inspect element'); inspectedElement = null; TestUtils.act(() => { - getPath(id, ['props', 'nestedObject', 'a']); + getInspectedElementPath(id, ['props', 'nestedObject', 'a']); jest.runOnlyPendingTimers(); }); expect(inspectedElement).not.toBeNull(); @@ -580,7 +588,7 @@ describe('InspectedElementContext', () => { inspectedElement = null; TestUtils.act(() => { - getPath(id, ['props', 'nestedObject', 'c']); + getInspectedElementPath(id, ['props', 'nestedObject', 'c']); jest.runOnlyPendingTimers(); }); expect(inspectedElement).not.toBeNull(); @@ -629,6 +637,7 @@ describe('InspectedElementContext', () => { ReactDOM.render( { const id = ((store.getElementIDAtIndex(0): any): number); - let getPath: GetPath = ((null: any): GetPath); + let getInspectedElementPath: GetInspectedElementPath = ((null: any): GetInspectedElementPath); let inspectedElement = null; function Suspender({ target }) { const context = React.useContext(InspectedElementContext); - getPath = context.getPath; - inspectedElement = context.read(id); + getInspectedElementPath = context.getInspectedElementPath; + inspectedElement = context.getInspectedElement(id); return null; } @@ -667,7 +676,7 @@ describe('InspectedElementContext', () => { ), false ); - expect(getPath).not.toBeNull(); + expect(getInspectedElementPath).not.toBeNull(); expect(inspectedElement).not.toBeNull(); expect(inspectedElement).toMatchSnapshot('1: Initially inspect element'); @@ -675,6 +684,7 @@ describe('InspectedElementContext', () => { ReactDOM.render( { inspectedElement = null; TestUtils.act(() => { - getPath(id, ['props', 'nestedObject', 'a']); + getInspectedElementPath(id, ['props', 'nestedObject', 'a']); jest.runOnlyPendingTimers(); }); expect(inspectedElement).not.toBeNull(); diff --git a/src/backend/legacy/renderer.js b/src/backend/legacy/renderer.js index 1c389a3f9a..2e22bc7337 100644 --- a/src/backend/legacy/renderer.js +++ b/src/backend/legacy/renderer.js @@ -552,6 +552,8 @@ export function attach( let currentlyInspectedElementID: number | null = null; let currentlyInspectedPaths: Object = {}; + // Track the intersection of currently inspected paths, + // so that we can send their data along if the element is re-rendered. function mergeInspectedPaths(path: Array) { let current = currentlyInspectedPaths; path.forEach(key => { @@ -563,6 +565,8 @@ export function attach( } function createIsPathWhitelisted(key: string) { + // This function helps prevent previously-inspected paths from being dehydrated in updates. + // This is important to avoid a bad user experience where expanded toggles collapse on update. return function isPathWhitelisted(path: Array): boolean { let current = currentlyInspectedPaths[key]; if (!current) { diff --git a/src/backend/renderer.js b/src/backend/renderer.js index e75a4548c1..244454c702 100644 --- a/src/backend/renderer.js +++ b/src/backend/renderer.js @@ -2137,6 +2137,8 @@ export function attach( ); } + // Track the intersection of currently inspected paths, + // so that we can send their data along if the element is re-rendered. function mergeInspectedPaths(path: Array) { let current = currentlyInspectedPaths; path.forEach(key => { @@ -2148,9 +2150,13 @@ export function attach( } function createIsPathWhitelisted(isHooksPath: boolean, key: string | null) { + // This function helps prevent previously-inspected paths from being dehydrated in updates. + // This is important to avoid a bad user experience where expanded toggles collapse on update. return function isPathWhitelisted(path: Array): boolean { // Dehydrating the 'subHooks' property makes the HooksTree UI a lot more complicated, // so it's easiest for now if we just don't break on this boundary. + // We can always dehydrate a level deeper (in the value object). + // TODO (hydration) This check depends on a LEVEL_THRESHOLD of 2 to avoid dehydrating a hook incorrectly. if (isHooksPath && path[path.length - 1] === 'subHooks') { return true; } @@ -2225,8 +2231,10 @@ export function attach( mergeInspectedPaths(path); } + // Clone before cleaning so that we preserve the full data. + // This will enable us to send patches without re-inspecting if hydrated paths are requested. + // (Reducing how often we shallow-render is a better DX for function components that use hooks.) const cleanedInspectedElement = { ...mostRecentlyInspectedElement }; - cleanedInspectedElement.context = cleanForBridge( cleanedInspectedElement.context, createIsPathWhitelisted(false, 'context') @@ -2308,7 +2316,6 @@ export function attach( const fiber = findCurrentFiberUsingSlowPathById(id); if (fiber !== null) { if (typeof overrideHookState === 'function') { - console.log('[renderer] overrideHookState()', { path, value, index }); overrideHookState(fiber, index, path, value); } } diff --git a/src/devtools/views/Components/HooksTree.js b/src/devtools/views/Components/HooksTree.js index a9c8122ff1..d1ab04778f 100644 --- a/src/devtools/views/Components/HooksTree.js +++ b/src/devtools/views/Components/HooksTree.js @@ -23,12 +23,12 @@ type HooksTreeViewProps = {| |}; export function HooksTreeView({ canEditHooks, hooks, id }: HooksTreeViewProps) { - const { getPath } = useContext(InspectedElementContext); + const { getInspectedElementPath } = useContext(InspectedElementContext); const inspectPath = useCallback( (path: Array) => { - getPath(id, ['hooks', ...path]); + getInspectedElementPath(id, ['hooks', ...path]); }, - [getPath, id] + [getInspectedElementPath, id] ); const handleCopy = useCallback(() => copy(serializeHooksForCopy(hooks)), [ hooks, @@ -114,7 +114,9 @@ function HookView({ if (hook.hasOwnProperty(meta.inspected)) { // This Hook is too deep and hasn't been hydrated. - // TODO (hydration) show UI to load its data. + if (__DEV__) { + console.warn('Unexpected dehydrated hook; this is a DevTools error.'); + } return (
@@ -124,8 +126,6 @@ function HookView({ ); } - // TODO Add click and key handlers for toggling element open/close state. - const isCustomHook = subHooks.length > 0; const type = typeof value; @@ -218,8 +218,10 @@ function HookView({ bridge.send('overrideHookState', { id, hookID, - // Hooks override function expects a relative path for the specified hook (id). - // This should not include the fake tree structure DevTools uses for display. + // Hooks override function expects a relative path for the specified hook (id), + // starting with its id within the (flat) hooks list structure. + // This relative path does not include the fake tree structure DevTools uses for display, + // so it's important that we remove that part of the path before sending the update. path: absolutePath.slice(path.length + 1), rendererID, value, diff --git a/src/devtools/views/Components/InspectedElementContext.js b/src/devtools/views/Components/InspectedElementContext.js index 65d5e6d421..97efcac574 100644 --- a/src/devtools/views/Components/InspectedElementContext.js +++ b/src/devtools/views/Components/InspectedElementContext.js @@ -26,12 +26,17 @@ import type { } from 'src/devtools/views/Components/types'; import type { Resource, Thenable } from '../../cache'; -export type GetPath = (id: number, path: Array) => void; -export type Read = (id: number) => InspectedElementFrontend | null; +export type GetInspectedElementPath = ( + id: number, + path: Array +) => void; +export type GetInspectedElement = ( + id: number +) => InspectedElementFrontend | null; type Context = {| - getPath: GetPath, - read: Read, + getInspectedElementPath: GetInspectedElementPath, + getInspectedElement: GetInspectedElement, |}; const InspectedElementContext = createContext(((null: any): Context)); @@ -76,7 +81,8 @@ function InspectedElementContextController({ children }: Props) { const bridge = useContext(BridgeContext); const store = useContext(StoreContext); - const getPath = useCallback( + // Ask the backend to fill in a "dehydrated" path; this will result in a "inspectedElement". + const getInspectedElementPath = useCallback( (id: number, path: Array) => { const rendererID = store.getRendererIDForElement(id); bridge.send('inspectElement', { id, path, rendererID }); @@ -84,7 +90,7 @@ function InspectedElementContextController({ children }: Props) { [bridge, store] ); - const read = useCallback( + const getInspectedElement = useCallback( (id: number) => { const element = store.getElementByID(id); if (element !== null) { @@ -266,10 +272,10 @@ function InspectedElementContextController({ children }: Props) { }, [bridge, selectedElementID, store]); const value = useMemo( - () => ({ getPath, read }), + () => ({ getInspectedElement, getInspectedElementPath }), // InspectedElement is used to invalidate the cache and schedule an update with React. // eslint-disable-next-line react-hooks/exhaustive-deps - [currentlyInspectedElement, getPath, read] + [currentlyInspectedElement, getInspectedElement, getInspectedElementPath] ); return ( @@ -289,6 +295,8 @@ function hydrateHelper( if (path) { const { length } = path; if (length > 0) { + // Hydration helper requires full paths, but inspection dehydrates with relative paths. + // In that event it's important that we adjust the "cleaned" paths to match. cleaned = cleaned.map(cleanedPath => cleanedPath.slice(length)); } } diff --git a/src/devtools/views/Components/KeyValue.js b/src/devtools/views/Components/KeyValue.js index 4d19c255c0..7fd333a0a8 100644 --- a/src/devtools/views/Components/KeyValue.js +++ b/src/devtools/views/Components/KeyValue.js @@ -22,9 +22,6 @@ type KeyValueProps = {| value: any, |}; -// TODO (hydration) Don't display meta objects. -// Add event listener to request a "read" instead. - export default function KeyValue({ depth, inspectPath, diff --git a/src/devtools/views/Components/SelectedElement.js b/src/devtools/views/Components/SelectedElement.js index 97cd2d20db..5802d204b9 100644 --- a/src/devtools/views/Components/SelectedElement.js +++ b/src/devtools/views/Components/SelectedElement.js @@ -25,7 +25,7 @@ import { import styles from './SelectedElement.css'; -import type { GetPath } from './InspectedElementContext'; +import type { GetInspectedElementPath } from './InspectedElementContext'; import type { Element, InspectedElement } from './types'; import type { ElementType } from 'src/types'; @@ -39,7 +39,9 @@ export default function SelectedElement(_: Props) { const store = useContext(StoreContext); const { dispatch: modalDialogDispatch } = useContext(ModalDialogContext); - const { getPath, read } = useContext(InspectedElementContext); + const { getInspectedElementPath, getInspectedElement } = useContext( + InspectedElementContext + ); const element = inspectedElementID !== null @@ -47,7 +49,7 @@ export default function SelectedElement(_: Props) { : null; const inspectedElement = - inspectedElementID != null ? read(inspectedElementID) : null; + inspectedElementID != null ? getInspectedElement(inspectedElementID) : null; const highlightElement = useCallback(() => { if (element !== null && inspectedElementID !== null) { @@ -202,10 +204,10 @@ export default function SelectedElement(_: Props) { {inspectedElement !== null && ( )} @@ -217,7 +219,7 @@ export type InspectPath = (path: Array) => void; type InspectedElementViewProps = {| element: Element, - getPath: GetPath, + getInspectedElementPath: GetInspectedElementPath, inspectedElement: InspectedElement, |}; @@ -225,7 +227,7 @@ const IS_SUSPENDED = 'Suspended'; function InspectedElementView({ element, - getPath, + getInspectedElementPath, inspectedElement, }: InspectedElementViewProps) { const { id, type } = element; @@ -247,21 +249,21 @@ function InspectedElementView({ const inspectContextPath = useCallback( (path: Array) => { - getPath(id, ['context', ...path]); + getInspectedElementPath(id, ['context', ...path]); }, - [getPath, id] + [getInspectedElementPath, id] ); const inspectPropsPath = useCallback( (path: Array) => { - getPath(id, ['props', ...path]); + getInspectedElementPath(id, ['props', ...path]); }, - [getPath, id] + [getInspectedElementPath, id] ); const inspectStatePath = useCallback( (path: Array) => { - getPath(id, ['state', ...path]); + getInspectedElementPath(id, ['state', ...path]); }, - [getPath, id] + [getInspectedElementPath, id] ); let overrideContextFn = null; diff --git a/src/hydration.js b/src/hydration.js index 6bd9750f25..438165c765 100644 --- a/src/hydration.js +++ b/src/hydration.js @@ -42,7 +42,8 @@ type Dehydrated = {| // Reducing this threshold will improve the speed of initial component inspection, // but may decrease the responsiveness of expanding objects/arrays to inspect further. // -// Note that reducing the threshold to below two effectively breaks the inspected hooks interface. +// Note that reducing the threshold to below 2 effectively breaks the inspected hooks interface. +// It is only safe to dehydrate hooks within the "value" key, never within the "subHooks" array directly. const LEVEL_THRESHOLD = 2; /**