From d85cf3e5ab6e049626a8bedddffbaec05c516195 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Wed, 5 Feb 2025 12:52:48 +0000 Subject: [PATCH] DevTools: refactor NativeStyleEditor, don't use custom cache implementation (#32298) We have this really old (5+ years) feature for inspecting native styles of React Native Host components. We also have a custom Cache implementation in React DevTools, which was forked from React at some point. We know that this should be removed, but it spans through critical parts of the application, like fetching and caching inspected element. Before this PR, this was also used for caching native style and layouts of RN Host components. This approach is out of date, and was based on the presence of Suspense boundary around inspected element View, which we have removed to speed up element inspection - https://github.com/facebook/react/pull/30555. Looks like I've introduced a regression in https://github.com/facebook/react/pull/31956: - Custom Cache implementation will throw thenables and suspend. - Because of this, some descendant Suspense boundaries will not resolve for a long time, and React will throw an error https://react.dev/errors/482. I've switched from a usage of this custom Cache implementation to a naive fetching in effect and keeping the layout and style in a local state of a Context, which will be propagated downwards. The race should be impossible, this is guaranteed by the mechanism for queueing messages through microtasks queue. The only downside is the UI. If you quickly switch between 2 elements, and one of them has native style, while the other doesn't, UI will feel jumpy. We can address this later with a Suspense boundary, if needed. --- .../__tests__/__e2e__/components.test.js | 34 ++--- .../NativeStyleEditor/LayoutViewer.css | 1 - .../NativeStyleEditor/StyleEditor.css | 1 - .../NativeStyleEditor/StyleEditor.js | 2 +- .../Components/NativeStyleEditor/context.js | 142 +++--------------- .../Components/NativeStyleEditor/index.css | 3 + .../Components/NativeStyleEditor/index.js | 39 +++-- .../devtools/views/Components/TreeContext.js | 17 ++- 8 files changed, 67 insertions(+), 172 deletions(-) create mode 100644 packages/react-devtools-shared/src/devtools/views/Components/NativeStyleEditor/index.css diff --git a/packages/react-devtools-inline/__tests__/__e2e__/components.test.js b/packages/react-devtools-inline/__tests__/__e2e__/components.test.js index 71c2e603ccb27..9a6b67d8ec65b 100644 --- a/packages/react-devtools-inline/__tests__/__e2e__/components.test.js +++ b/packages/react-devtools-inline/__tests__/__e2e__/components.test.js @@ -212,8 +212,8 @@ test.describe('Components', () => { }); test('should allow searching for component by name', async () => { - async function getComponentSearchResultsCount() { - return await page.evaluate(() => { + async function waitForComponentSearchResultsCount(text) { + return await page.waitForFunction(expectedElementText => { const {createTestNameSelector, findAllNodes} = window.REACT_DOM_DEVTOOLS; const container = document.getElementById('devtools'); @@ -221,8 +221,10 @@ test.describe('Components', () => { const element = findAllNodes(container, [ createTestNameSelector('ComponentSearchInput-ResultsCount'), ])[0]; - return element.innerText; - }); + return element !== undefined + ? element.innerText === expectedElementText + : false; + }, text); } async function focusComponentSearch() { @@ -238,35 +240,27 @@ test.describe('Components', () => { await focusComponentSearch(); await page.keyboard.insertText('List'); - let count = await getComponentSearchResultsCount(); - expect(count).toBe('1 | 4'); + await waitForComponentSearchResultsCount('1 | 4'); await page.keyboard.insertText('Item'); - count = await getComponentSearchResultsCount(); - expect(count).toBe('1 | 3'); + await waitForComponentSearchResultsCount('1 | 3'); await page.keyboard.press('Enter'); - count = await getComponentSearchResultsCount(); - expect(count).toBe('2 | 3'); + await waitForComponentSearchResultsCount('2 | 3'); await page.keyboard.press('Enter'); - count = await getComponentSearchResultsCount(); - expect(count).toBe('3 | 3'); + await waitForComponentSearchResultsCount('3 | 3'); await page.keyboard.press('Enter'); - count = await getComponentSearchResultsCount(); - expect(count).toBe('1 | 3'); + await waitForComponentSearchResultsCount('1 | 3'); await page.keyboard.press('Shift+Enter'); - count = await getComponentSearchResultsCount(); - expect(count).toBe('3 | 3'); + await waitForComponentSearchResultsCount('3 | 3'); await page.keyboard.press('Shift+Enter'); - count = await getComponentSearchResultsCount(); - expect(count).toBe('2 | 3'); + await waitForComponentSearchResultsCount('2 | 3'); await page.keyboard.press('Shift+Enter'); - count = await getComponentSearchResultsCount(); - expect(count).toBe('1 | 3'); + await waitForComponentSearchResultsCount('1 | 3'); }); }); diff --git a/packages/react-devtools-shared/src/devtools/views/Components/NativeStyleEditor/LayoutViewer.css b/packages/react-devtools-shared/src/devtools/views/Components/NativeStyleEditor/LayoutViewer.css index d21b49197a19b..44bc93f88c2d5 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/NativeStyleEditor/LayoutViewer.css +++ b/packages/react-devtools-shared/src/devtools/views/Components/NativeStyleEditor/LayoutViewer.css @@ -1,6 +1,5 @@ .LayoutViewer { padding: 0.25rem; - border-top: 1px solid var(--color-border); font-family: var(--font-family-monospace); font-size: var(--font-size-monospace-small); } diff --git a/packages/react-devtools-shared/src/devtools/views/Components/NativeStyleEditor/StyleEditor.css b/packages/react-devtools-shared/src/devtools/views/Components/NativeStyleEditor/StyleEditor.css index fcb3a3122c5fe..bf7a8b27fa8a1 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/NativeStyleEditor/StyleEditor.css +++ b/packages/react-devtools-shared/src/devtools/views/Components/NativeStyleEditor/StyleEditor.css @@ -2,7 +2,6 @@ font-family: var(--font-family-monospace); font-size: var(--font-size-monospace-normal); padding: 0.25rem; - border-top: 1px solid var(--color-border); } .HeaderRow { diff --git a/packages/react-devtools-shared/src/devtools/views/Components/NativeStyleEditor/StyleEditor.js b/packages/react-devtools-shared/src/devtools/views/Components/NativeStyleEditor/StyleEditor.js index b1416f6e89f95..2bb9aec2219b2 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/NativeStyleEditor/StyleEditor.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/NativeStyleEditor/StyleEditor.js @@ -81,7 +81,7 @@ export default function StyleEditor({id, style}: Props): React.Node { {keys.length > 0 && keys.map(attribute => ( StyleAndLayoutFrontend | null; -type Context = { - getStyleAndLayout: GetStyleAndLayout, -}; +type Context = StyleAndLayoutFrontend | null; const NativeStyleContext: ReactContext = createContext( ((null: any): Context), ); NativeStyleContext.displayName = 'NativeStyleContext'; -type ResolveFn = (styleAndLayout: StyleAndLayoutFrontend) => void; -type InProgressRequest = { - promise: Thenable, - resolveFn: ResolveFn, -}; - -const inProgressRequests: WeakMap = new WeakMap(); -const resource: Resource = - createResource( - (element: Element) => { - const request = inProgressRequests.get(element); - if (request != null) { - return request.promise; - } - - let resolveFn: - | ResolveFn - | (( - result: Promise | StyleAndLayoutFrontend, - ) => void) = ((null: any): ResolveFn); - const promise = new Promise(resolve => { - resolveFn = resolve; - }); - - inProgressRequests.set(element, ({promise, resolveFn}: $FlowFixMe)); - - return (promise: $FlowFixMe); - }, - (element: Element) => element, - {useWeakMap: true}, - ); - type Props = { children: React$Node, }; @@ -86,72 +37,22 @@ type Props = { function NativeStyleContextController({children}: Props): React.Node { const bridge = useContext(BridgeContext); const store = useContext(StoreContext); - - const getStyleAndLayout = useCallback( - (id: number) => { - const element = store.getElementByID(id); - if (element !== null) { - return resource.read(element); - } else { - return null; - } - }, - [store], - ); - - // It's very important that this context consumes inspectedElementID and not NativeStyleID. - // Otherwise the effect that sends the "inspect" message across the bridge- - // would itself be blocked by the same render that suspends (waiting for the data). const {inspectedElementID} = useContext(TreeStateContext); const [currentStyleAndLayout, setCurrentStyleAndLayout] = useState(null); - // This effect handler invalidates the suspense cache and schedules rendering updates with React. - useEffect(() => { - const onStyleAndLayout = ({id, layout, style}: StyleAndLayoutBackend) => { - const element = store.getElementByID(id); - if (element !== null) { - const styleAndLayout: StyleAndLayoutFrontend = { - layout, - style, - }; - const request = inProgressRequests.get(element); - if (request != null) { - inProgressRequests.delete(element); - request.resolveFn(styleAndLayout); - setCurrentStyleAndLayout(styleAndLayout); - } else { - resource.write(element, styleAndLayout); - - // Schedule update with React if the currently-selected element has been invalidated. - if (id === inspectedElementID) { - setCurrentStyleAndLayout(styleAndLayout); - } - } - } - }; - - bridge.addListener('NativeStyleEditor_styleAndLayout', onStyleAndLayout); - return () => - bridge.removeListener( - 'NativeStyleEditor_styleAndLayout', - onStyleAndLayout, - ); - }, [bridge, currentStyleAndLayout, inspectedElementID, store]); - // This effect handler polls for updates on the currently selected element. useEffect(() => { if (inspectedElementID === null) { + setCurrentStyleAndLayout(null); return () => {}; } - const rendererID = store.getRendererIDForElement(inspectedElementID); - - let timeoutID: TimeoutID | null = null; - + let requestTimeoutId: TimeoutID | null = null; const sendRequest = () => { - timeoutID = null; + requestTimeoutId = null; + const rendererID = store.getRendererIDForElement(inspectedElementID); if (rendererID !== null) { bridge.send('NativeStyleEditor_measure', { @@ -165,38 +66,37 @@ function NativeStyleContextController({children}: Props): React.Node { // We'll poll for an update in the response handler below. sendRequest(); - const onStyleAndLayout = ({id}: StyleAndLayoutBackend) => { + const onStyleAndLayout = ({id, layout, style}: StyleAndLayoutBackend) => { // If this is the element we requested, wait a little bit and then ask for another update. if (id === inspectedElementID) { - if (timeoutID !== null) { - clearTimeout(timeoutID); + if (requestTimeoutId !== null) { + clearTimeout(requestTimeoutId); } - timeoutID = setTimeout(sendRequest, 1000); + requestTimeoutId = setTimeout(sendRequest, 1000); } + + const styleAndLayout: StyleAndLayoutFrontend = { + layout, + style, + }; + setCurrentStyleAndLayout(styleAndLayout); }; bridge.addListener('NativeStyleEditor_styleAndLayout', onStyleAndLayout); - return () => { bridge.removeListener( 'NativeStyleEditor_styleAndLayout', onStyleAndLayout, ); - if (timeoutID !== null) { - clearTimeout(timeoutID); + if (requestTimeoutId !== null) { + clearTimeout(requestTimeoutId); } }; }, [bridge, inspectedElementID, store]); - const value = useMemo( - () => ({getStyleAndLayout}), - // NativeStyle is used to invalidate the cache and schedule an update with React. - [currentStyleAndLayout, getStyleAndLayout], - ); - return ( - + {children} ); diff --git a/packages/react-devtools-shared/src/devtools/views/Components/NativeStyleEditor/index.css b/packages/react-devtools-shared/src/devtools/views/Components/NativeStyleEditor/index.css new file mode 100644 index 0000000000000..d4b1ad2be824e --- /dev/null +++ b/packages/react-devtools-shared/src/devtools/views/Components/NativeStyleEditor/index.css @@ -0,0 +1,3 @@ +.Stack > *:not(:first-child) { + border-top: 1px solid var(--color-border); +} diff --git a/packages/react-devtools-shared/src/devtools/views/Components/NativeStyleEditor/index.js b/packages/react-devtools-shared/src/devtools/views/Components/NativeStyleEditor/index.js index 10bf8023966b8..bf3f6a8e9f30c 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/NativeStyleEditor/index.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/NativeStyleEditor/index.js @@ -8,19 +8,19 @@ */ import * as React from 'react'; -import {Fragment, useContext, useMemo} from 'react'; +import {useContext, useMemo} from 'react'; + import {StoreContext} from 'react-devtools-shared/src/devtools/views/context'; import {useSubscription} from 'react-devtools-shared/src/devtools/views/hooks'; +import {TreeStateContext} from 'react-devtools-shared/src/devtools/views/Components/TreeContext'; + import {NativeStyleContext} from './context'; import LayoutViewer from './LayoutViewer'; import StyleEditor from './StyleEditor'; -import {TreeStateContext} from '../TreeContext'; - -type Props = {}; +import styles from './index.css'; -export default function NativeStyleEditorWrapper(_: Props): React.Node { +export default function NativeStyleEditorWrapper(): React.Node { const store = useContext(StoreContext); - const subscription = useMemo( () => ({ getCurrentValue: () => store.supportsNativeStyleEditor, @@ -33,8 +33,8 @@ export default function NativeStyleEditorWrapper(_: Props): React.Node { }), [store], ); - const supportsNativeStyleEditor = useSubscription(subscription); + const supportsNativeStyleEditor = useSubscription(subscription); if (!supportsNativeStyleEditor) { return null; } @@ -42,32 +42,27 @@ export default function NativeStyleEditorWrapper(_: Props): React.Node { return ; } -function NativeStyleEditor(_: Props) { - const {getStyleAndLayout} = useContext(NativeStyleContext); - +function NativeStyleEditor() { const {inspectedElementID} = useContext(TreeStateContext); + const inspectedElementStyleAndLayout = useContext(NativeStyleContext); if (inspectedElementID === null) { return null; } - - const maybeStyleAndLayout = getStyleAndLayout(inspectedElementID); - if (maybeStyleAndLayout === null) { + if (inspectedElementStyleAndLayout === null) { return null; } - const {layout, style} = maybeStyleAndLayout; + const {layout, style} = inspectedElementStyleAndLayout; + if (layout === null && style === null) { + return null; + } return ( - +
{layout !== null && ( )} - {style !== null && ( - - )} - + {style !== null && } +
); } diff --git a/packages/react-devtools-shared/src/devtools/views/Components/TreeContext.js b/packages/react-devtools-shared/src/devtools/views/Components/TreeContext.js index c9013e8011f68..ab4d9332d735f 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/TreeContext.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/TreeContext.js @@ -35,6 +35,7 @@ import { useMemo, useReducer, useRef, + startTransition, } from 'react'; import {createRegExp} from '../utils'; import {StoreContext} from '../context'; @@ -890,15 +891,19 @@ function TreeContextController({ ? store.getIndexOfElementID(store.lastSelectedHostInstanceElementId) : null, }); + const dispatchWrapper = useMemo( + () => (action: Action) => startTransition(() => dispatch(action)), + [dispatch], + ); // Listen for host element selections. useEffect(() => { const handler = (id: Element['id']) => - dispatch({type: 'SELECT_ELEMENT_BY_ID', payload: id}); + dispatchWrapper({type: 'SELECT_ELEMENT_BY_ID', payload: id}); store.addListener('hostInstanceSelected', handler); return () => store.removeListener('hostInstanceSelected', handler); - }, [store, dispatch]); + }, [store, 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. @@ -922,7 +927,7 @@ function TreeContextController({ Array, Map, ]) => { - dispatch({ + dispatchWrapper({ type: 'HANDLE_STORE_MUTATION', payload: [addedElementIDs, removedElementIDs], }); @@ -933,7 +938,7 @@ function TreeContextController({ // 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 Map()], }); @@ -941,11 +946,11 @@ function TreeContextController({ store.addListener('mutated', handleStoreMutated); return () => store.removeListener('mutated', handleStoreMutated); - }, [dispatch, initialRevision, store]); + }, [dispatchWrapper, initialRevision, store]); return ( - + {children}