From bfe91fbecf183f85fc1c4f909e12a6833a247319 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Wed, 9 Oct 2024 13:57:02 +0100 Subject: [PATCH] refactor[react-devtools]: flatten reload and profile config (#31132) Stacked on https://github.com/facebook/react/pull/31131. See last commit. This is a clean-up and a pre-requisite for next changes: 1. `ReloadAndProfileConfig` is now split into boolean value and settings object. This is mainly because I will add one more setting soon, and also because settings might be persisted for a longer time than the flag which signals if the Backend was reloaded for profiling. Ideally, this settings should probably be moved to the global Hook object, same as we did for console patching. 2. Host is now responsible for reseting the cached values, Backend will execute provided `onReloadAndProfileFlagsReset` callback. --- packages/react-devtools-core/src/backend.js | 41 ++++++++--- .../src/contentScripts/backendManager.js | 15 +++- .../src/contentScripts/installHook.js | 13 +++- packages/react-devtools-inline/src/backend.js | 14 +++- .../src/attachRenderer.js | 8 ++- .../src/backend/agent.js | 33 +++------ .../src/backend/fiber/renderer.js | 11 ++- .../src/backend/types.js | 12 +--- packages/react-devtools-shared/src/hook.js | 15 ++-- packages/react-devtools-shared/src/utils.js | 68 +++++++++---------- 10 files changed, 131 insertions(+), 99 deletions(-) diff --git a/packages/react-devtools-core/src/backend.js b/packages/react-devtools-core/src/backend.js index 54a6b9b48a..9305155ab0 100644 --- a/packages/react-devtools-core/src/backend.js +++ b/packages/react-devtools-core/src/backend.js @@ -26,8 +26,7 @@ import type { import type { DevToolsHook, DevToolsHookSettings, - ReloadAndProfileConfig, - ReloadAndProfileConfigPersistence, + ProfilingSettings, } from 'react-devtools-shared/src/backend/types'; import type {ResolveNativeStyle} from 'react-devtools-shared/src/backend/NativeStyleEditor/setupNativeStyleEditor'; @@ -42,7 +41,9 @@ type ConnectOptions = { websocket?: ?WebSocket, onSettingsUpdated?: (settings: $ReadOnly) => void, isReloadAndProfileSupported?: boolean, - reloadAndProfileConfigPersistence?: ReloadAndProfileConfigPersistence, + isProfiling?: boolean, + onReloadAndProfile?: (recordChangeDescriptions: boolean) => void, + onReloadAndProfileFlagsReset?: () => void, }; let savedComponentFilters: Array = @@ -63,9 +64,15 @@ export function initialize( maybeSettingsOrSettingsPromise?: | DevToolsHookSettings | Promise, - reloadAndProfileConfig?: ReloadAndProfileConfig, + shouldStartProfilingNow: boolean = false, + profilingSettings?: ProfilingSettings, ) { - installHook(window, maybeSettingsOrSettingsPromise, reloadAndProfileConfig); + installHook( + window, + maybeSettingsOrSettingsPromise, + shouldStartProfilingNow, + profilingSettings, + ); } export function connectToDevTools(options: ?ConnectOptions) { @@ -86,7 +93,9 @@ export function connectToDevTools(options: ?ConnectOptions) { isAppActive = () => true, onSettingsUpdated, isReloadAndProfileSupported = getIsReloadAndProfileSupported(), - reloadAndProfileConfigPersistence, + isProfiling, + onReloadAndProfile, + onReloadAndProfileFlagsReset, } = options || {}; const protocol = useHttps ? 'wss' : 'ws'; @@ -180,7 +189,11 @@ export function connectToDevTools(options: ?ConnectOptions) { // TODO (npm-packages) Warn if "isBackendStorageAPISupported" // $FlowFixMe[incompatible-call] found when upgrading Flow - const agent = new Agent(bridge, reloadAndProfileConfigPersistence); + const agent = new Agent(bridge, isProfiling, onReloadAndProfile); + if (typeof onReloadAndProfileFlagsReset === 'function') { + onReloadAndProfileFlagsReset(); + } + if (onSettingsUpdated != null) { agent.addListener('updateHookSettings', onSettingsUpdated); } @@ -320,7 +333,9 @@ type ConnectWithCustomMessagingOptions = { resolveRNStyle?: ResolveNativeStyle, onSettingsUpdated?: (settings: $ReadOnly) => void, isReloadAndProfileSupported?: boolean, - reloadAndProfileConfigPersistence?: ReloadAndProfileConfigPersistence, + isProfiling?: boolean, + onReloadAndProfile?: (recordChangeDescriptions: boolean) => void, + onReloadAndProfileFlagsReset?: () => void, }; export function connectWithCustomMessagingProtocol({ @@ -331,7 +346,9 @@ export function connectWithCustomMessagingProtocol({ resolveRNStyle, onSettingsUpdated, isReloadAndProfileSupported = getIsReloadAndProfileSupported(), - reloadAndProfileConfigPersistence, + isProfiling, + onReloadAndProfile, + onReloadAndProfileFlagsReset, }: ConnectWithCustomMessagingOptions): Function { const hook: ?DevToolsHook = window.__REACT_DEVTOOLS_GLOBAL_HOOK__; if (hook == null) { @@ -368,7 +385,11 @@ export function connectWithCustomMessagingProtocol({ bridge.send('overrideComponentFilters', savedComponentFilters); } - const agent = new Agent(bridge, reloadAndProfileConfigPersistence); + const agent = new Agent(bridge, isProfiling, onReloadAndProfile); + if (typeof onReloadAndProfileFlagsReset === 'function') { + onReloadAndProfileFlagsReset(); + } + if (onSettingsUpdated != null) { agent.addListener('updateHookSettings', onSettingsUpdated); } diff --git a/packages/react-devtools-extensions/src/contentScripts/backendManager.js b/packages/react-devtools-extensions/src/contentScripts/backendManager.js index 63519b506d..402a137857 100644 --- a/packages/react-devtools-extensions/src/contentScripts/backendManager.js +++ b/packages/react-devtools-extensions/src/contentScripts/backendManager.js @@ -14,6 +14,11 @@ import type { import {hasAssignedBackend} from 'react-devtools-shared/src/backend/utils'; import {COMPACT_VERSION_NAME} from 'react-devtools-extensions/src/utils'; import {getIsReloadAndProfileSupported} from 'react-devtools-shared/src/utils'; +import { + getIfReloadedAndProfiling, + onReloadAndProfile, + onReloadAndProfileFlagsReset, +} from 'react-devtools-shared/src/utils'; let welcomeHasInitialized = false; const requiredBackends = new Set(); @@ -140,7 +145,15 @@ function activateBackend(version: string, hook: DevToolsHook) { }, }); - const agent = new Agent(bridge); + const agent = new Agent( + bridge, + getIfReloadedAndProfiling(), + onReloadAndProfile, + ); + // Agent read flags successfully, we can count it as successful launch + // Clean up flags, so that next reload won't start profiling + onReloadAndProfileFlagsReset(); + agent.addListener('shutdown', () => { // If we received 'shutdown' from `agent`, we assume the `bridge` is already shutting down, // and that caused the 'shutdown' event on the `agent`, so we don't need to call `bridge.shutdown()` here. diff --git a/packages/react-devtools-extensions/src/contentScripts/installHook.js b/packages/react-devtools-extensions/src/contentScripts/installHook.js index b7b96ed247..e70e97b285 100644 --- a/packages/react-devtools-extensions/src/contentScripts/installHook.js +++ b/packages/react-devtools-extensions/src/contentScripts/installHook.js @@ -1,4 +1,8 @@ import {installHook} from 'react-devtools-shared/src/hook'; +import { + getIfReloadedAndProfiling, + getProfilingSettings, +} from 'react-devtools-shared/src/utils'; let resolveHookSettingsInjection; @@ -34,8 +38,15 @@ if (!window.hasOwnProperty('__REACT_DEVTOOLS_GLOBAL_HOOK__')) { payload: {handshake: true}, }); + const shouldStartProfiling = getIfReloadedAndProfiling(); + const profilingSettings = getProfilingSettings(); // Can't delay hook installation, inject settings lazily - installHook(window, hookSettingsPromise); + installHook( + window, + hookSettingsPromise, + shouldStartProfiling, + profilingSettings, + ); // Detect React window.__REACT_DEVTOOLS_GLOBAL_HOOK__.on( diff --git a/packages/react-devtools-inline/src/backend.js b/packages/react-devtools-inline/src/backend.js index 41af7809be..354970446d 100644 --- a/packages/react-devtools-inline/src/backend.js +++ b/packages/react-devtools-inline/src/backend.js @@ -8,7 +8,12 @@ import setupNativeStyleEditor from 'react-devtools-shared/src/backend/NativeStyl import type {BackendBridge} from 'react-devtools-shared/src/bridge'; import type {Wall} from 'react-devtools-shared/src/frontend/types'; -import {getIsReloadAndProfileSupported} from 'react-devtools-shared/src/utils'; +import { + getIfReloadedAndProfiling, + getIsReloadAndProfileSupported, + onReloadAndProfile, + onReloadAndProfileFlagsReset, +} from 'react-devtools-shared/src/utils'; function startActivation(contentWindow: any, bridge: BackendBridge) { const onSavedPreferences = (data: $FlowFixMe) => { @@ -63,7 +68,12 @@ function startActivation(contentWindow: any, bridge: BackendBridge) { } function finishActivation(contentWindow: any, bridge: BackendBridge) { - const agent = new Agent(bridge); + const agent = new Agent( + bridge, + getIfReloadedAndProfiling(), + onReloadAndProfile, + ); + onReloadAndProfileFlagsReset(); const hook = contentWindow.__REACT_DEVTOOLS_GLOBAL_HOOK__; if (hook) { diff --git a/packages/react-devtools-shared/src/attachRenderer.js b/packages/react-devtools-shared/src/attachRenderer.js index cd7a348b65..fedf76293c 100644 --- a/packages/react-devtools-shared/src/attachRenderer.js +++ b/packages/react-devtools-shared/src/attachRenderer.js @@ -12,8 +12,8 @@ import type { RendererInterface, DevToolsHook, RendererID, + ProfilingSettings, } from 'react-devtools-shared/src/backend/types'; -import type {ReloadAndProfileConfig} from './backend/types'; import {attach as attachFlight} from 'react-devtools-shared/src/backend/flight/renderer'; import {attach as attachFiber} from 'react-devtools-shared/src/backend/fiber/renderer'; @@ -30,7 +30,8 @@ export default function attachRenderer( id: RendererID, renderer: ReactRenderer, global: Object, - reloadAndProfileConfig: ReloadAndProfileConfig, + shouldStartProfilingNow: boolean, + profilingSettings: ProfilingSettings, ): RendererInterface | void { // only attach if the renderer is compatible with the current version of the backend if (!isMatchingRender(renderer.reconcilerVersion || renderer.version)) { @@ -55,7 +56,8 @@ export default function attachRenderer( id, renderer, global, - reloadAndProfileConfig, + shouldStartProfilingNow, + profilingSettings, ); } else if (renderer.ComponentTree) { // react-dom v15 diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js index f5fde694e7..12704899ec 100644 --- a/packages/react-devtools-shared/src/backend/agent.js +++ b/packages/react-devtools-shared/src/backend/agent.js @@ -26,11 +26,9 @@ import type { RendererID, RendererInterface, DevToolsHookSettings, - ReloadAndProfileConfigPersistence, } from './types'; import type {ComponentFilter} from 'react-devtools-shared/src/frontend/types'; import {isReactNativeEnvironment} from './utils'; -import {defaultReloadAndProfileConfigPersistence} from '../utils'; import { sessionStorageGetItem, sessionStorageRemoveItem, @@ -151,33 +149,21 @@ export default class Agent extends EventEmitter<{ }> { _bridge: BackendBridge; _isProfiling: boolean = false; - _recordChangeDescriptions: boolean = false; _rendererInterfaces: {[key: RendererID]: RendererInterface, ...} = {}; _persistedSelection: PersistedSelection | null = null; _persistedSelectionMatch: PathMatch | null = null; _traceUpdatesEnabled: boolean = false; - _reloadAndProfileConfigPersistence: ReloadAndProfileConfigPersistence; + _onReloadAndProfile: ((recordChangeDescriptions: boolean) => void) | void; constructor( bridge: BackendBridge, - reloadAndProfileConfigPersistence?: ReloadAndProfileConfigPersistence = defaultReloadAndProfileConfigPersistence, + isProfiling: boolean = false, + onReloadAndProfile?: (recordChangeDescriptions: boolean) => void, ) { super(); - this._reloadAndProfileConfigPersistence = reloadAndProfileConfigPersistence; - const {getReloadAndProfileConfig, setReloadAndProfileConfig} = - reloadAndProfileConfigPersistence; - const reloadAndProfileConfig = getReloadAndProfileConfig(); - if (reloadAndProfileConfig.shouldReloadAndProfile) { - this._recordChangeDescriptions = - reloadAndProfileConfig.recordChangeDescriptions; - this._isProfiling = true; - - setReloadAndProfileConfig({ - shouldReloadAndProfile: false, - recordChangeDescriptions: false, - }); - } + this._isProfiling = isProfiling; + this._onReloadAndProfile = onReloadAndProfile; const persistedSelectionString = sessionStorageGetItem( SESSION_STORAGE_LAST_SELECTION_KEY, @@ -674,10 +660,9 @@ export default class Agent extends EventEmitter<{ reloadAndProfile: (recordChangeDescriptions: boolean) => void = recordChangeDescriptions => { - this._reloadAndProfileConfigPersistence.setReloadAndProfileConfig({ - shouldReloadAndProfile: true, - recordChangeDescriptions, - }); + if (typeof this._onReloadAndProfile === 'function') { + this._onReloadAndProfile(recordChangeDescriptions); + } // This code path should only be hit if the shell has explicitly told the Store that it supports profiling. // In that case, the shell must also listen for this specific message to know when it needs to reload the app. @@ -757,7 +742,6 @@ export default class Agent extends EventEmitter<{ startProfiling: (recordChangeDescriptions: boolean) => void = recordChangeDescriptions => { - this._recordChangeDescriptions = recordChangeDescriptions; this._isProfiling = true; for (const rendererID in this._rendererInterfaces) { const renderer = ((this._rendererInterfaces[ @@ -770,7 +754,6 @@ export default class Agent extends EventEmitter<{ stopProfiling: () => void = () => { this._isProfiling = false; - this._recordChangeDescriptions = false; for (const rendererID in this._rendererInterfaces) { const renderer = ((this._rendererInterfaces[ (rendererID: any) diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index 91fe467a95..57aca225b8 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -104,7 +104,6 @@ import { supportsOwnerStacks, supportsConsoleTasks, } from './DevToolsFiberComponentStack'; -import type {ReloadAndProfileConfig} from '../types'; // $FlowFixMe[method-unbinding] const toString = Object.prototype.toString; @@ -136,6 +135,7 @@ import type { WorkTagMap, CurrentDispatcherRef, LegacyDispatcherRef, + ProfilingSettings, } from '../types'; import type { ComponentFilter, @@ -864,7 +864,8 @@ export function attach( rendererID: number, renderer: ReactRenderer, global: Object, - reloadAndProfileConfig: ReloadAndProfileConfig, + shouldStartProfilingNow: boolean, + profilingSettings: ProfilingSettings, ): RendererInterface { // Newer versions of the reconciler package also specific reconciler version. // If that version number is present, use it. @@ -5225,10 +5226,8 @@ export function attach( } // Automatically start profiling so that we don't miss timing info from initial "mount". - if (reloadAndProfileConfig.shouldReloadAndProfile) { - const shouldRecordChangeDescriptions = - reloadAndProfileConfig.recordChangeDescriptions; - startProfiling(shouldRecordChangeDescriptions); + if (shouldStartProfilingNow) { + startProfiling(profilingSettings.recordChangeDescriptions); } function getNearestFiber(devtoolsInstance: DevToolsInstance): null | Fiber { diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index c6f743546e..61546f2c28 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -485,20 +485,10 @@ export type DevToolsBackend = { setupNativeStyleEditor?: SetupNativeStyleEditor, }; -export type ReloadAndProfileConfig = { - shouldReloadAndProfile: boolean, +export type ProfilingSettings = { recordChangeDescriptions: boolean, }; -// Linter doesn't speak Flow's `Partial` type -// eslint-disable-next-line no-undef -type PartialReloadAndProfileConfig = Partial; - -export type ReloadAndProfileConfigPersistence = { - setReloadAndProfileConfig: (config: PartialReloadAndProfileConfig) => void, - getReloadAndProfileConfig: () => ReloadAndProfileConfig, -}; - export type DevToolsHook = { listeners: {[key: string]: Array, ...}, rendererInterfaces: Map, diff --git a/packages/react-devtools-shared/src/hook.js b/packages/react-devtools-shared/src/hook.js index 3b45c7417d..4aa6518cd1 100644 --- a/packages/react-devtools-shared/src/hook.js +++ b/packages/react-devtools-shared/src/hook.js @@ -16,7 +16,7 @@ import type { RendererInterface, DevToolsBackend, DevToolsHookSettings, - ReloadAndProfileConfig, + ProfilingSettings, } from './backend/types'; import { @@ -27,7 +27,6 @@ import { import attachRenderer from './attachRenderer'; import formatConsoleArguments from 'react-devtools-shared/src/backend/utils/formatConsoleArguments'; import formatWithStyles from 'react-devtools-shared/src/backend/utils/formatWithStyles'; -import {defaultReloadAndProfileConfigPersistence} from './utils'; // React's custom built component stack strings match "\s{4}in" // Chrome's prefix matches "\s{4}at" @@ -51,12 +50,17 @@ function areStackTracesEqual(a: string, b: string): boolean { const targetConsole: Object = console; +const defaultProfilingSettings: ProfilingSettings = { + recordChangeDescriptions: false, +}; + export function installHook( target: any, maybeSettingsOrSettingsPromise?: | DevToolsHookSettings | Promise, - reloadAndProfileConfig?: ReloadAndProfileConfig = defaultReloadAndProfileConfigPersistence.getReloadAndProfileConfig(), + shouldStartProfilingNow: boolean = false, + profilingSettings: ProfilingSettings = defaultProfilingSettings, ): DevToolsHook | null { if (target.hasOwnProperty('__REACT_DEVTOOLS_GLOBAL_HOOK__')) { return null; @@ -195,6 +199,8 @@ export function installHook( } catch (err) {} } + // TODO: isProfiling should be stateful, and we should update it once profiling is finished + const isProfiling = shouldStartProfilingNow; let uidCounter = 0; function inject(renderer: ReactRenderer): number { const id = ++uidCounter; @@ -215,7 +221,8 @@ export function installHook( id, renderer, target, - reloadAndProfileConfig, + isProfiling, + profilingSettings, ); if (rendererInterface != null) { hook.rendererInterfaces.set(id, rendererInterface); diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index 715834334f..d3f18920fc 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -56,8 +56,9 @@ import { localStorageGetItem, localStorageSetItem, sessionStorageGetItem, + sessionStorageRemoveItem, sessionStorageSetItem, -} from './storage'; +} from 'react-devtools-shared/src/storage'; import {meta} from './hydration'; import isArray from './isArray'; @@ -67,12 +68,11 @@ import type { SerializedElement as SerializedElementFrontend, LRUCache, } from 'react-devtools-shared/src/frontend/types'; -import type {SerializedElement as SerializedElementBackend} from 'react-devtools-shared/src/backend/types'; -import {isSynchronousXHRSupported} from './backend/utils'; import type { - ReloadAndProfileConfig, - ReloadAndProfileConfigPersistence, -} from './backend/types'; + ProfilingSettings, + SerializedElement as SerializedElementBackend, +} from 'react-devtools-shared/src/backend/types'; +import {isSynchronousXHRSupported} from './backend/utils'; // $FlowFixMe[method-unbinding] const hasOwnProperty = Object.prototype.hasOwnProperty; @@ -990,34 +990,30 @@ export function getIsReloadAndProfileSupported(): boolean { return isBackendStorageAPISupported && isSynchronousXHRSupported(); } -export const defaultReloadAndProfileConfigPersistence: ReloadAndProfileConfigPersistence = - { - setReloadAndProfileConfig({ - shouldReloadAndProfile, - recordChangeDescriptions, - }): void { - if (shouldReloadAndProfile != null) { - sessionStorageSetItem( - SESSION_STORAGE_RELOAD_AND_PROFILE_KEY, - shouldReloadAndProfile ? 'true' : 'false', - ); - } - if (recordChangeDescriptions != null) { - sessionStorageSetItem( - SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY, - recordChangeDescriptions ? 'true' : 'false', - ); - } - }, - getReloadAndProfileConfig(): ReloadAndProfileConfig { - return { - shouldReloadAndProfile: - sessionStorageGetItem(SESSION_STORAGE_RELOAD_AND_PROFILE_KEY) === - 'true', - recordChangeDescriptions: - sessionStorageGetItem( - SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY, - ) === 'true', - }; - }, +// Expected to be used only by browser extension and react-devtools-inline +export function getIfReloadedAndProfiling(): boolean { + return ( + sessionStorageGetItem(SESSION_STORAGE_RELOAD_AND_PROFILE_KEY) === 'true' + ); +} + +export function getProfilingSettings(): ProfilingSettings { + return { + recordChangeDescriptions: + sessionStorageGetItem(SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY) === + 'true', }; +} + +export function onReloadAndProfile(recordChangeDescriptions: boolean): void { + sessionStorageSetItem(SESSION_STORAGE_RELOAD_AND_PROFILE_KEY, 'true'); + sessionStorageSetItem( + SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY, + recordChangeDescriptions ? 'true' : 'false', + ); +} + +export function onReloadAndProfileFlagsReset(): void { + sessionStorageRemoveItem(SESSION_STORAGE_RELOAD_AND_PROFILE_KEY); + sessionStorageRemoveItem(SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY); +}