From dbd42d406ce86af30eae97a52df728e3aebb1db2 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Thu, 9 Nov 2023 11:09:10 -0800 Subject: [PATCH] refactor: use reactDevToolsAgent in state (#41291) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/41291 Changelog: [Internal] We have the same logic in 3 different places, encapsulating access to React DevTools hook in one place, all other components that require agent will get it as a prop. Reviewed By: GijsWeterings Differential Revision: D50559550 fbshipit-source-id: f3667a82ca48a36032c60c730df8f661098401ee --- .../TraceUpdateOverlay/TraceUpdateOverlay.js | 61 ++++----- .../Libraries/Inspector/Inspector.js | 50 ++----- ...oolsOverlay.js => ReactDevToolsOverlay.js} | 124 +++++++++--------- .../Libraries/ReactNative/AppContainer-dev.js | 39 ++++-- .../Libraries/Types/ReactDevToolsTypes.js | 7 + 5 files changed, 128 insertions(+), 153 deletions(-) rename packages/react-native/Libraries/Inspector/{DevtoolsOverlay.js => ReactDevToolsOverlay.js} (66%) diff --git a/packages/react-native/Libraries/Components/TraceUpdateOverlay/TraceUpdateOverlay.js b/packages/react-native/Libraries/Components/TraceUpdateOverlay/TraceUpdateOverlay.js index c4e7627bc60..57c70fd5f28 100644 --- a/packages/react-native/Libraries/Components/TraceUpdateOverlay/TraceUpdateOverlay.js +++ b/packages/react-native/Libraries/Components/TraceUpdateOverlay/TraceUpdateOverlay.js @@ -11,7 +11,6 @@ import type { InstanceFromReactDevTools, ReactDevToolsAgent, - ReactDevToolsGlobalHook, } from '../../Types/ReactDevToolsTypes'; import type {Overlay} from './TraceUpdateOverlayNativeComponent'; @@ -26,47 +25,27 @@ import TraceUpdateOverlayNativeComponent, { import * as React from 'react'; const {useEffect, useRef, useState} = React; -const hook: ReactDevToolsGlobalHook = window.__REACT_DEVTOOLS_GLOBAL_HOOK__; const isNativeComponentReady = Platform.OS === 'android' && UIManager.hasViewManagerConfig('TraceUpdateOverlay'); -let devToolsAgent: ?ReactDevToolsAgent; -export default function TraceUpdateOverlay(): React.Node { +type Props = { + reactDevToolsAgent: ReactDevToolsAgent, +}; + +export default function TraceUpdateOverlay({ + reactDevToolsAgent, +}: Props): React.Node { const [overlayDisabled, setOverlayDisabled] = useState(false); - // This effect is designed to be explicitly shown here to avoid re-subscribe from the same - // overlay component. + useEffect(() => { if (!isNativeComponentReady) { return; } - function attachToDevtools(agent: ReactDevToolsAgent) { - devToolsAgent = agent; - agent.addListener('drawTraceUpdates', onAgentDrawTraceUpdates); - agent.addListener('disableTraceUpdates', onAgentDisableTraceUpdates); - } - - function subscribe() { - hook?.on('react-devtools', attachToDevtools); - if (hook?.reactDevtoolsAgent) { - attachToDevtools(hook.reactDevtoolsAgent); - } - } - - function unsubscribe() { - hook?.off('react-devtools', attachToDevtools); - const agent = devToolsAgent; - if (agent != null) { - agent.removeListener('drawTraceUpdates', onAgentDrawTraceUpdates); - agent.removeListener('disableTraceUpdates', onAgentDisableTraceUpdates); - devToolsAgent = null; - } - } - - function onAgentDrawTraceUpdates( + const drawTraceUpdates = ( nodesToDraw: Array<{node: InstanceFromReactDevTools, color: string}> = [], - ) { + ) => { // If overlay is disabled before, now it's enabled. setOverlayDisabled(false); @@ -114,16 +93,24 @@ export default function TraceUpdateOverlay(): React.Node { console.error(`Failed to measure updated traces. Error: ${err}`); }, ); - } + }; - function onAgentDisableTraceUpdates() { + const disableTraceUpdates = () => { // When trace updates are disabled from the backend, we won't receive draw events until it's enabled by the next draw. We can safely remove the overlay as it's not needed now. setOverlayDisabled(true); - } + }; - subscribe(); - return unsubscribe; - }, []); // Only run once when the overlay initially rendered + reactDevToolsAgent.addListener('drawTraceUpdates', drawTraceUpdates); + reactDevToolsAgent.addListener('disableTraceUpdates', drawTraceUpdates); + + return () => { + reactDevToolsAgent.removeListener('drawTraceUpdates', drawTraceUpdates); + reactDevToolsAgent.removeListener( + 'disableTraceUpdates', + disableTraceUpdates, + ); + }; + }, [reactDevToolsAgent]); const nativeComponentRef = useRef>(null); diff --git a/packages/react-native/Libraries/Inspector/Inspector.js b/packages/react-native/Libraries/Inspector/Inspector.js index 330b501b0a7..7fb5acee9a9 100644 --- a/packages/react-native/Libraries/Inspector/Inspector.js +++ b/packages/react-native/Libraries/Inspector/Inspector.js @@ -11,6 +11,7 @@ 'use strict'; import type {TouchedViewDataAtPoint} from '../Renderer/shims/ReactNativeTypes'; +import type {ReactDevToolsAgent} from '../Types/ReactDevToolsTypes'; import type {HostRef} from './getInspectorDataForViewAtPoint'; const ReactNativeStyleAttributes = require('../Components/View/ReactNativeStyleAttributes'); @@ -36,14 +37,15 @@ if (hook) { ); } +type Props = { + inspectedView: ?HostRef, + onRequestRerenderApp: () => void, + reactDevToolsAgent?: ReactDevToolsAgent, +}; + class Inspector extends React.Component< + Props, { - inspectedView: ?HostRef, - onRequestRerenderApp: () => void, - ... - }, - { - devtoolsAgent: ?Object, hierarchy: any, panelPos: string, inspecting: boolean, @@ -59,11 +61,10 @@ class Inspector extends React.Component< _subs: ?Array<() => void>; _setTouchedViewData: ?(TouchedViewDataAtPoint) => void; - constructor(props: Object) { + constructor(props: Props) { super(props); this.state = { - devtoolsAgent: null, hierarchy: null, panelPos: 'bottom', inspecting: true, @@ -75,43 +76,18 @@ class Inspector extends React.Component< }; } - componentDidMount() { - hook.on('react-devtools', this._attachToDevtools); - // if devtools is already started - if (hook.reactDevtoolsAgent) { - this._attachToDevtools(hook.reactDevtoolsAgent); - } - } - componentWillUnmount() { if (this._subs) { this._subs.map(fn => fn()); } - hook.off('react-devtools', this._attachToDevtools); + this._setTouchedViewData = null; } - UNSAFE_componentWillReceiveProps(newProps: Object) { + UNSAFE_componentWillReceiveProps(newProps: Props) { this.setState({inspectedView: newProps.inspectedView}); } - _attachToDevtools = (agent: Object) => { - agent.addListener('shutdown', this._onAgentShutdown); - - this.setState({ - devtoolsAgent: agent, - }); - }; - - _onAgentShutdown = () => { - const agent = this.state.devtoolsAgent; - if (agent != null) { - agent.removeListener('shutdown', this._onAgentShutdown); - - this.setState({devtoolsAgent: null}); - } - }; - setSelection(i: number) { const hierarchyItem = this.state.hierarchy[i]; // we pass in findNodeHandle as the method is injected @@ -146,7 +122,7 @@ class Inspector extends React.Component< // Sync the touched view with React DevTools. // Note: This is Paper only. To support Fabric, // DevTools needs to be updated to not rely on view tags. - const agent = this.state.devtoolsAgent; + const agent = this.props.reactDevToolsAgent; if (agent) { agent.selectNode(findNodeHandle(touchedViewTag)); if (closestInstance != null) { @@ -226,7 +202,7 @@ class Inspector extends React.Component< )} (null); const [isInspecting, setIsInspecting] = useState(false); - const devToolsAgentRef = useRef(null); useEffect(() => { - let devToolsAgent = null; let hideTimeoutId = null; function onAgentHideNativeHighlight() { @@ -49,16 +53,16 @@ export default function DevtoolsOverlay({ }, 100); } - function onAgentShowNativeHighlight(node: any) { + function onAgentShowNativeHighlight(node?: InstanceFromReactDevTools) { clearTimeout(hideTimeoutId); // `canonical.publicInstance` => Fabric // `canonical` => Legacy Fabric // `node` => Legacy renderer const component = - (node.canonical && node.canonical.publicInstance) ?? + (node?.canonical && node.canonical.publicInstance) ?? // TODO: remove this check when syncing the new version of the renderer from React to React Native. - node.canonical ?? + node?.canonical ?? node; if (!component || !component.measure) { return; @@ -72,28 +76,23 @@ export default function DevtoolsOverlay({ } function cleanup() { - const currentAgent = devToolsAgent; - if (currentAgent != null) { - currentAgent.removeListener( - 'hideNativeHighlight', - onAgentHideNativeHighlight, - ); - currentAgent.removeListener( - 'showNativeHighlight', - onAgentShowNativeHighlight, - ); - currentAgent.removeListener('shutdown', cleanup); - currentAgent.removeListener( - 'startInspectingNative', - onStartInspectingNative, - ); - currentAgent.removeListener( - 'stopInspectingNative', - onStopInspectingNative, - ); - devToolsAgent = null; - } - devToolsAgentRef.current = null; + reactDevToolsAgent.removeListener( + 'hideNativeHighlight', + onAgentHideNativeHighlight, + ); + reactDevToolsAgent.removeListener( + 'showNativeHighlight', + onAgentShowNativeHighlight, + ); + reactDevToolsAgent.removeListener('shutdown', cleanup); + reactDevToolsAgent.removeListener( + 'startInspectingNative', + onStartInspectingNative, + ); + reactDevToolsAgent.removeListener( + 'stopInspectingNative', + onStopInspectingNative, + ); } function onStartInspectingNative() { @@ -104,40 +103,37 @@ export default function DevtoolsOverlay({ setIsInspecting(false); } - function _attachToDevtools(agent: Object) { - devToolsAgent = agent; - devToolsAgentRef.current = agent; - agent.addListener('hideNativeHighlight', onAgentHideNativeHighlight); - agent.addListener('showNativeHighlight', onAgentShowNativeHighlight); - agent.addListener('shutdown', cleanup); - agent.addListener('startInspectingNative', onStartInspectingNative); - agent.addListener('stopInspectingNative', onStopInspectingNative); - } + reactDevToolsAgent.addListener( + 'hideNativeHighlight', + onAgentHideNativeHighlight, + ); + reactDevToolsAgent.addListener( + 'showNativeHighlight', + onAgentShowNativeHighlight, + ); + reactDevToolsAgent.addListener('shutdown', cleanup); + reactDevToolsAgent.addListener( + 'startInspectingNative', + onStartInspectingNative, + ); + reactDevToolsAgent.addListener( + 'stopInspectingNative', + onStopInspectingNative, + ); - hook.on('react-devtools', _attachToDevtools); - if (hook.reactDevtoolsAgent) { - _attachToDevtools(hook.reactDevtoolsAgent); - } - return () => { - hook.off('react-devtools', _attachToDevtools); - cleanup(); - }; - }, []); + return cleanup; + }, [reactDevToolsAgent]); const findViewForLocation = useCallback( (x: number, y: number) => { - const agent = devToolsAgentRef.current; - if (agent == null) { - return; - } getInspectorDataForViewAtPoint(inspectedView, x, y, viewData => { const {touchedViewTag, closestInstance, frame} = viewData; if (closestInstance != null || touchedViewTag != null) { // We call `selectNode` for both non-fabric(viewTag) and fabric(instance), // this makes sure it works for both architectures. - agent.selectNode(findNodeHandle(touchedViewTag)); + reactDevToolsAgent.selectNode(findNodeHandle(touchedViewTag)); if (closestInstance != null) { - agent.selectNode(closestInstance); + reactDevToolsAgent.selectNode(closestInstance); } setInspected({ frame, @@ -147,18 +143,14 @@ export default function DevtoolsOverlay({ return false; }); }, - [inspectedView], + [inspectedView, reactDevToolsAgent], ); const stopInspecting = useCallback(() => { - const agent = devToolsAgentRef.current; - if (agent == null) { - return; - } - agent.stopInspectingNative(true); + reactDevToolsAgent.stopInspectingNative(true); setIsInspecting(false); setInspected(null); - }, []); + }, [reactDevToolsAgent]); const onPointerMove = useCallback( (e: PointerEvent) => { @@ -200,6 +192,7 @@ export default function DevtoolsOverlay({ onResponderMove: onResponderMove, onResponderRelease: stopInspecting, }; + return ( ); } + return highlight; } diff --git a/packages/react-native/Libraries/ReactNative/AppContainer-dev.js b/packages/react-native/Libraries/ReactNative/AppContainer-dev.js index 7cced77b069..0d0c15359c4 100644 --- a/packages/react-native/Libraries/ReactNative/AppContainer-dev.js +++ b/packages/react-native/Libraries/ReactNative/AppContainer-dev.js @@ -9,13 +9,17 @@ * @oncall react_native */ +import type { + ReactDevToolsAgent, + ReactDevToolsGlobalHook, +} from '../Types/ReactDevToolsTypes'; import type {Props} from './AppContainer'; import TraceUpdateOverlay from '../Components/TraceUpdateOverlay/TraceUpdateOverlay'; import View from '../Components/View/View'; import ViewNativeComponent from '../Components/View/ViewNativeComponent'; import RCTDeviceEventEmitter from '../EventEmitter/RCTDeviceEventEmitter'; -import ReactDevToolsOverlay from '../Inspector/DevtoolsOverlay'; +import ReactDevToolsOverlay from '../Inspector/ReactDevToolsOverlay'; import LogBoxNotificationContainer from '../LogBox/LogBoxNotificationContainer'; import StyleSheet from '../StyleSheet/StyleSheet'; import {RootTagContext, createRootTag} from './RootTag'; @@ -23,16 +27,19 @@ import * as React from 'react'; const {useEffect, useState, useCallback} = React; -const reactDevToolsHook = window.__REACT_DEVTOOLS_GLOBAL_HOOK__; +const reactDevToolsHook: ReactDevToolsGlobalHook = + window.__REACT_DEVTOOLS_GLOBAL_HOOK__; type InspectorDeferredProps = { inspectedView: React.ElementRef | null, onInspectedViewRerenderRequest: () => void, + reactDevToolsAgent?: ReactDevToolsAgent, }; const InspectorDeferred = ({ inspectedView, onInspectedViewRerenderRequest, + reactDevToolsAgent, }: InspectorDeferredProps) => { // D39382967 adds a require cycle: InitializeCore -> AppContainer -> Inspector -> InspectorPanel -> ScrollView -> InitializeCore // We can't remove it yet, fallback to dynamic require for now. This is the only reason why this logic is in a separate function. @@ -42,6 +49,7 @@ const InspectorDeferred = ({ ); }; @@ -62,8 +70,8 @@ const AppContainer = ({ const [key, setKey] = useState(0); const [shouldRenderInspector, setShouldRenderInspector] = useState(false); - const [shouldRenderDebuggingOverlays, setShouldRenderDebuggingOverlays] = - useState(reactDevToolsHook?.reactDevtoolsAgent != null); + const [reactDevToolsAgent, setReactDevToolsAgent] = + useState(reactDevToolsHook?.reactDevtoolsAgent); useEffect(() => { let inspectorSubscription = null; @@ -75,12 +83,9 @@ const AppContainer = ({ } let reactDevToolsAgentListener = null; - // Subscribe listener, if agent is not attached yet - if ( - reactDevToolsHook != null && - reactDevToolsHook.reactDevtoolsAgent == null - ) { - reactDevToolsAgentListener = () => setShouldRenderDebuggingOverlays(true); + // If this is first render, subscribe to the event from React DevTools hook + if (reactDevToolsHook != null && reactDevToolsAgent == null) { + reactDevToolsAgentListener = setReactDevToolsAgent; reactDevToolsHook.on?.('react-devtools', reactDevToolsAgentListener); } @@ -99,7 +104,7 @@ const AppContainer = ({ let innerView: React.Node = ( {innerView} - {shouldRenderDebuggingOverlays && } - {shouldRenderDebuggingOverlays && ( - + {reactDevToolsAgent != null && ( + + )} + {reactDevToolsAgent != null && ( + )} {shouldRenderInspector && ( )} diff --git a/packages/react-native/Libraries/Types/ReactDevToolsTypes.js b/packages/react-native/Libraries/Types/ReactDevToolsTypes.js index 60a84ec821a..0fb8ccbda66 100644 --- a/packages/react-native/Libraries/Types/ReactDevToolsTypes.js +++ b/packages/react-native/Libraries/Types/ReactDevToolsTypes.js @@ -28,10 +28,17 @@ export type InstanceFromReactDevTools = export type ReactDevToolsAgentEvents = { drawTraceUpdates: [Array<{node: InstanceFromReactDevTools, color: string}>], disableTraceUpdates: [], + + showNativeHighlight: [node: InstanceFromReactDevTools], + hideNativeHighlight: [], + shutdown: [], + startInspectingNative: [], + stopInspectingNative: [], }; export type ReactDevToolsAgent = { selectNode(node: mixed): void, + stopInspectingNative(value: boolean): void, addListener>( event: Event, listener: (...ReactDevToolsAgentEvents[Event]) => void,