From 35f2ab70f918cf0afe7cfd9233b86664e9ae2507 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 5 Jan 2021 16:50:36 -0500 Subject: [PATCH] DevTools: Replace legacy Suspense cache with unstable_getCacheForType --- .../src/devtools/cache.js | 2 + .../views/Components/InspectedElement.js | 4 + .../Components/InspectedElementContext.js | 162 ++++++++++++------ .../InspectedElementErrorsAndWarningsTree.js | 40 ++--- 4 files changed, 131 insertions(+), 77 deletions(-) diff --git a/packages/react-devtools-shared/src/devtools/cache.js b/packages/react-devtools-shared/src/devtools/cache.js index af6a02faa0cf2..573f666402717 100644 --- a/packages/react-devtools-shared/src/devtools/cache.js +++ b/packages/react-devtools-shared/src/devtools/cache.js @@ -12,6 +12,8 @@ import type {Thenable} from 'shared/ReactTypes'; import * as React from 'react'; import {createContext} from 'react'; +// TODO (cache) Remove this cache; it is outdated and will not work with newer APIs like startTransition. + // Cache implementation was forked from the React repo: // https://github.com/facebook/react/blob/master/packages/react-cache/src/ReactCache.js // diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js index f0ecc32be3770..4a16932a384be 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js @@ -39,6 +39,8 @@ export default function InspectedElementWrapper(_: Props) { const {dispatch: modalDialogDispatch} = useContext(ModalDialogContext); const { + clearErrorsForInspectedElement, + clearWarningsForInspectedElement, copyInspectedElementPath, getInspectedElementPath, getInspectedElement, @@ -228,6 +230,8 @@ export default function InspectedElementWrapper(_: Props) { key={ inspectedElementID /* Force reset when selected Element changes */ } + clearErrorsForInspectedElement={clearErrorsForInspectedElement} + clearWarningsForInspectedElement={clearWarningsForInspectedElement} copyInspectedElementPath={copyInspectedElementPath} element={element} getInspectedElementPath={getInspectedElementPath} diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContext.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContext.js index 01b05a9fa33a1..c2ade23edd31b 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContext.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContext.js @@ -10,6 +10,9 @@ import * as React from 'react'; import { createContext, + unstable_getCacheForType as getCacheForType, + unstable_startTransition as startTransition, + unstable_useCacheRefresh as useCacheRefresh, useCallback, useContext, useEffect, @@ -17,8 +20,6 @@ import { useRef, useState, } from 'react'; -import {unstable_batchedUpdates as batchedUpdates} from 'react-dom'; -import {createResource} from '../../cache'; import {BridgeContext, StoreContext} from '../context'; import {hydrate, fillInPath} from 'react-devtools-shared/src/hydration'; import {TreeStateContext} from './TreeContext'; @@ -33,7 +34,6 @@ import type { Element, InspectedElement as InspectedElementFrontend, } from 'react-devtools-shared/src/devtools/views/Components/types'; -import type {Resource, Thenable} from '../../cache'; export type StoreAsGlobal = (id: number, path: Array) => void; @@ -51,13 +51,15 @@ export type GetInspectedElement = ( id: number, ) => InspectedElementFrontend | null; -type RefreshInspectedElement = () => void; +type ClearErrorsForInspectedElement = () => void; +type ClearWarningsForInspectedElement = () => void; export type InspectedElementContextType = {| + clearErrorsForInspectedElement: ClearErrorsForInspectedElement, + clearWarningsForInspectedElement: ClearWarningsForInspectedElement, copyInspectedElementPath: CopyInspectedElementPath, getInspectedElementPath: GetInspectedElementPath, getInspectedElement: GetInspectedElement, - refreshInspectedElement: RefreshInspectedElement, storeAsGlobal: StoreAsGlobal, |}; @@ -67,35 +69,68 @@ const InspectedElementContext = createContext( InspectedElementContext.displayName = 'InspectedElementContext'; type ResolveFn = (inspectedElement: InspectedElementFrontend) => void; -type InProgressRequest = {| - promise: Thenable, - resolveFn: ResolveFn, +type Callback = (inspectedElement: InspectedElementFrontend) => void; +type Thenable = {| + callbacks: Set, + then: (callback: Callback) => void, + resolve: ResolveFn, |}; -const inProgressRequests: WeakMap = new WeakMap(); -const resource: Resource< - Element, - Element, - InspectedElementFrontend, -> = createResource( - (element: Element) => { - const request = inProgressRequests.get(element); - if (request != null) { - return request.promise; - } +const inspectedElementThenables: WeakMap = new WeakMap(); - let resolveFn = ((null: any): ResolveFn); - const promise = new Promise(resolve => { - resolveFn = resolve; - }); +type InspectedElementCache = WeakMap; - inProgressRequests.set(element, {promise, resolveFn}); +function createInspectedElementCache(): InspectedElementCache { + return new WeakMap(); +} - return promise; - }, - (element: Element) => element, - {useWeakMap: true}, -); +function getInspectedElementCache(): InspectedElementCache { + return getCacheForType(createInspectedElementCache); +} + +function setInspectedElement( + element: Element, + inspectedElement: InspectedElementFrontend, + inspectedElementCache: InspectedElementCache, +): void { + // TODO (cache) This mutation seems sketchy. + // Probably need to refresh the cache with a new seed. + inspectedElementCache.set(element, inspectedElement); + + const maybeThenable = inspectedElementThenables.get(element); + if (maybeThenable != null) { + inspectedElementThenables.delete(element); + + maybeThenable.resolve(inspectedElement); + } +} + +function getInspectedElement(element: Element): InspectedElementFrontend { + const inspectedElementCache = getInspectedElementCache(); + const maybeInspectedElement = inspectedElementCache.get(element); + if (maybeInspectedElement !== undefined) { + return maybeInspectedElement; + } + + const maybeThenable = inspectedElementThenables.get(element); + if (maybeThenable != null) { + throw maybeThenable; + } + + const thenable: Thenable = { + callbacks: new Set(), + then: callback => { + thenable.callbacks.add(callback); + }, + resolve: inspectedElement => { + thenable.callbacks.forEach(callback => callback(inspectedElement)); + }, + }; + + inspectedElementThenables.set(element, thenable); + + throw thenable; +} type Props = {| children: React$Node, @@ -145,14 +180,13 @@ function InspectedElementContextController({children}: Props) { [bridge, store], ); - const getInspectedElement = useCallback( + const getInspectedElementWrapper = useCallback( (id: number) => { const element = store.getElementByID(id); if (element !== null) { - return resource.read(element); - } else { - return null; + return getInspectedElement(element); } + return null; }, [store], ); @@ -162,11 +196,32 @@ function InspectedElementContextController({children}: Props) { // would itself be blocked by the same render that suspends (waiting for the data). const {selectedElementID} = useContext(TreeStateContext); - const refreshInspectedElement = useCallback(() => { + const refresh = useCacheRefresh(); + + const clearErrorsForInspectedElement = useCallback(() => { if (selectedElementID !== null) { const rendererID = store.getRendererIDForElement(selectedElementID); if (rendererID !== null) { bridge.send('inspectElement', {id: selectedElementID, rendererID}); + + startTransition(() => { + store.clearErrorsForElement(selectedElementID); + refresh(); + }); + } + } + }, [bridge, selectedElementID]); + + const clearWarningsForInspectedElement = useCallback(() => { + if (selectedElementID !== null) { + const rendererID = store.getRendererIDForElement(selectedElementID); + if (rendererID !== null) { + bridge.send('inspectElement', {id: selectedElementID, rendererID}); + + startTransition(() => { + store.clearWarningsForElement(selectedElementID); + refresh(); + }); } } }, [bridge, selectedElementID]); @@ -176,6 +231,8 @@ function InspectedElementContextController({children}: Props) { setCurrentlyInspectedElement, ] = useState(null); + const inspectedElementCache = getInspectedElementCache(); + // This effect handler invalidates the suspense cache and schedules rendering updates with React. useEffect(() => { const onInspectedElement = (data: InspectedElementPayload) => { @@ -198,7 +255,11 @@ function InspectedElementContextController({children}: Props) { fillInPath(inspectedElement, data.value, data.path, value); - resource.write(element, inspectedElement); + setInspectedElement( + element, + inspectedElement, + inspectedElementCache, + ); // Schedule update with React if the currently-selected element has been invalidated. if (id === selectedElementID) { @@ -277,20 +338,15 @@ function InspectedElementContextController({children}: Props) { element = store.getElementByID(id); if (element !== null) { - const request = inProgressRequests.get(element); - if (request != null) { - inProgressRequests.delete(element); - batchedUpdates(() => { - request.resolveFn(inspectedElement); - setCurrentlyInspectedElement(inspectedElement); - }); - } else { - resource.write(element, inspectedElement); - - // Schedule update with React if the currently-selected element has been invalidated. - if (id === selectedElementID) { - setCurrentlyInspectedElement(inspectedElement); - } + setInspectedElement( + element, + inspectedElement, + inspectedElementCache, + ); + + // Schedule update with React if the currently-selected element has been invalidated. + if (id === selectedElementID) { + setCurrentlyInspectedElement(inspectedElement); } } break; @@ -356,19 +412,21 @@ function InspectedElementContextController({children}: Props) { const value = useMemo( () => ({ + clearErrorsForInspectedElement, + clearWarningsForInspectedElement, copyInspectedElementPath, - getInspectedElement, + getInspectedElement: getInspectedElementWrapper, getInspectedElementPath, - refreshInspectedElement, storeAsGlobal, }), // InspectedElement is used to invalidate the cache and schedule an update with React. [ + clearErrorsForInspectedElement, + clearWarningsForInspectedElement, copyInspectedElementPath, currentlyInspectedElement, getInspectedElement, getInspectedElementPath, - refreshInspectedElement, storeAsGlobal, ], ); diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorsAndWarningsTree.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorsAndWarningsTree.js index 1f161c98836b6..f3144dd69c5cc 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorsAndWarningsTree.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorsAndWarningsTree.js @@ -8,7 +8,7 @@ */ import * as React from 'react'; -import {useContext} from 'react'; +import {useContext, unstable_useTransition as useTransition} from 'react'; import Button from '../Button'; import ButtonIcon from '../ButtonIcon'; import Store from '../../store'; @@ -31,7 +31,10 @@ export default function InspectedElementErrorsAndWarningsTree({ inspectedElement, store, }: Props) { - const {refreshInspectedElement} = useContext(InspectedElementContext); + const { + clearErrorsForInspectedElement, + clearWarningsForInspectedElement, + } = useContext(InspectedElementContext); const {showInlineWarningsAndErrors} = useContext(SettingsContext); if (!showInlineWarningsAndErrors) { @@ -40,26 +43,6 @@ export default function InspectedElementErrorsAndWarningsTree({ const {errors, warnings} = inspectedElement; - const clearErrors = () => { - const {id} = inspectedElement; - store.clearErrorsForElement(id); - - // Immediately poll for updated data. - // This avoids a delay between clicking the clear button and refreshing errors. - // Ideally this would be done with useTranstion but that requires updating to a newer Cache strategy. - refreshInspectedElement(); - }; - - const clearWarnings = () => { - const {id} = inspectedElement; - store.clearWarningsForElement(id); - - // Immediately poll for updated data. - // This avoids a delay between clicking the clear button and refreshing warnings. - // Ideally this would be done with useTranstion but that requires updating to a newer Cache strategy. - refreshInspectedElement(); - }; - return ( {errors.length > 0 && ( @@ -67,7 +50,7 @@ export default function InspectedElementErrorsAndWarningsTree({ badgeClassName={styles.ErrorBadge} bridge={bridge} className={styles.ErrorTree} - clearMessages={clearErrors} + clearMessages={clearErrorsForInspectedElement} entries={errors} label="errors" messageClassName={styles.Error} @@ -78,7 +61,7 @@ export default function InspectedElementErrorsAndWarningsTree({ badgeClassName={styles.WarningBadge} bridge={bridge} className={styles.WarningTree} - clearMessages={clearWarnings} + clearMessages={clearWarningsForInspectedElement} entries={warnings} label="warnings" messageClassName={styles.Warning} @@ -107,6 +90,8 @@ function Tree({ label, messageClassName, }: TreeProps) { + const [startTransition, isPending] = useTransition(); + if (entries.length === 0) { return null; } @@ -115,7 +100,12 @@ function Tree({
{label}