From 6a1bd52d478a6a99d76102074462c7b902ecceaa Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Thu, 4 May 2023 17:34:57 +0100 Subject: [PATCH] DevTools: fix backend activation (#26779) ## Summary We have a case: 1. Open components tab 2. Close Chrome / Firefox devtools window completely 3. Reopen browser devtools panel 4. Open components tab Currently, in version 4.27.6, we cannot load the components tree. This PR contains two changes: - non-functional refactoring in `react-devtools-shared/src/devtools/store.js`: removed some redundant type castings. - fixed backend manager logic (introduced in https://github.com/facebook/react/pull/26615) to activate already registered backends. Looks like frontend of devtools also depends on `renderer-attached` event, without it component tree won't load. ## How did you test this change? This fixes the case mentioned prior. Currently in 4.27.6 version it is not working, we need to refresh the page to make it work. I've tested this in several environments: chrome, firefox, standalone with RN application. --- .../src/backendManager.js | 7 + .../src/background.js | 7 +- .../src/backend/index.js | 8 +- .../src/backend/types.js | 2 - .../src/devtools/store.js | 124 ++++++++++-------- 5 files changed, 82 insertions(+), 66 deletions(-) diff --git a/packages/react-devtools-extensions/src/backendManager.js b/packages/react-devtools-extensions/src/backendManager.js index 23c6be57bc031..e5728ffd0f89b 100644 --- a/packages/react-devtools-extensions/src/backendManager.js +++ b/packages/react-devtools-extensions/src/backendManager.js @@ -61,6 +61,13 @@ function setup(hook: ?DevToolsHook) { hook.renderers.forEach(renderer => { registerRenderer(renderer); }); + + // Activate and remove from required all present backends, registered within the hook + hook.backends.forEach((_, backendVersion) => { + requiredBackends.delete(backendVersion); + activateBackend(backendVersion, hook); + }); + updateRequiredBackends(); // register renderers that inject themselves later. diff --git a/packages/react-devtools-extensions/src/background.js b/packages/react-devtools-extensions/src/background.js index 1d154575d5ba6..302f55ae8e531 100644 --- a/packages/react-devtools-extensions/src/background.js +++ b/packages/react-devtools-extensions/src/background.js @@ -28,9 +28,10 @@ async function dynamicallyInjectContentScripts() { // For some reason dynamically injected scripts might be already registered // Registering them again will fail, which will result into // __REACT_DEVTOOLS_GLOBAL_HOOK__ hook not being injected - await chrome.scripting.unregisterContentScripts({ - ids: contentScriptsToInject.map(s => s.id), - }); + + // Not specifying ids, because Chrome throws an error + // if id of non-injected script is provided + await chrome.scripting.unregisterContentScripts(); // equivalent logic for Firefox is in prepareInjection.js // Manifest V3 method of injecting content script diff --git a/packages/react-devtools-shared/src/backend/index.js b/packages/react-devtools-shared/src/backend/index.js index d3c194602eae8..376a23b369866 100644 --- a/packages/react-devtools-shared/src/backend/index.js +++ b/packages/react-devtools-shared/src/backend/index.js @@ -15,7 +15,7 @@ import {hasAssignedBackend} from './utils'; import type {DevToolsHook, ReactRenderer, RendererInterface} from './types'; -// this is the backend that is compactible with all older React versions +// this is the backend that is compatible with all older React versions function isMatchingRender(version: string): boolean { return !hasAssignedBackend(version); } @@ -31,6 +31,7 @@ export function initBackend( // DevTools didn't get injected into this page (maybe b'c of the contentType). return () => {}; } + const subs = [ hook.sub( 'renderer-attached', @@ -64,10 +65,6 @@ export function initBackend( ]; const attachRenderer = (id: number, renderer: ReactRenderer) => { - // skip if already attached - if (renderer.attached) { - return; - } // only attach if the renderer is compatible with the current version of the backend if (!isMatchingRender(renderer.reconcilerVersion || renderer.version)) { return; @@ -102,7 +99,6 @@ export function initBackend( } else { hook.emit('unsupported-renderer-version', id); } - renderer.attached = true; }; // Connect renderers that have already injected themselves. diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index ab859e71fc707..758ce827cf27f 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -171,8 +171,6 @@ export type ReactRenderer = { // 18.0+ injectProfilingHooks?: (profilingHooks: DevToolsProfilingHooks) => void, getLaneLabelMap?: () => Map | null, - // set by backend after successful attaching - attached?: boolean, ... }; diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js index eb5ac7af4f290..2c1316cba1e64 100644 --- a/packages/react-devtools-shared/src/devtools/store.js +++ b/packages/react-devtools-shared/src/devtools/store.js @@ -169,7 +169,7 @@ export default class Store extends EventEmitter<{ // Renderer ID is needed to support inspection fiber props, state, and hooks. _rootIDToRendererID: Map = new Map(); - // These options may be initially set by a confiugraiton option when constructing the Store. + // These options may be initially set by a configuration option when constructing the Store. _supportsNativeInspection: boolean = true; _supportsProfiling: boolean = false; _supportsReloadAndProfile: boolean = false; @@ -486,7 +486,7 @@ export default class Store extends EventEmitter<{ } containsElement(id: number): boolean { - return this._idToElement.get(id) != null; + return this._idToElement.has(id); } getElementAtIndex(index: number): Element | null { @@ -539,13 +539,13 @@ export default class Store extends EventEmitter<{ } getElementIDAtIndex(index: number): number | null { - const element: Element | null = this.getElementAtIndex(index); + const element = this.getElementAtIndex(index); return element === null ? null : element.id; } getElementByID(id: number): Element | null { const element = this._idToElement.get(id); - if (element == null) { + if (element === undefined) { console.warn(`No element found with id "${id}"`); return null; } @@ -607,7 +607,10 @@ export default class Store extends EventEmitter<{ let currentID = element.parentID; let index = 0; while (true) { - const current = ((this._idToElement.get(currentID): any): Element); + const current = this._idToElement.get(currentID); + if (current === undefined) { + return null; + } const {children} = current; for (let i = 0; i < children.length; i++) { @@ -615,7 +618,12 @@ export default class Store extends EventEmitter<{ if (childID === previousID) { break; } - const child = ((this._idToElement.get(childID): any): Element); + + const child = this._idToElement.get(childID); + if (child === undefined) { + return null; + } + index += child.isCollapsed ? 1 : child.weight; } @@ -637,7 +645,12 @@ export default class Store extends EventEmitter<{ if (rootID === currentID) { break; } - const root = ((this._idToElement.get(rootID): any): Element); + + const root = this._idToElement.get(rootID); + if (root === undefined) { + return null; + } + index += root.weight; } @@ -647,7 +660,7 @@ export default class Store extends EventEmitter<{ getOwnersListForElement(ownerID: number): Array { const list: Array = []; const element = this._idToElement.get(ownerID); - if (element != null) { + if (element !== undefined) { list.push({ ...element, depth: 0, @@ -665,8 +678,8 @@ export default class Store extends EventEmitter<{ // Seems better to defer the cost, since the set of ids is probably pretty small. const sortedIDs = Array.from(unsortedIDs).sort( (idA, idB) => - ((this.getIndexOfElementID(idA): any): number) - - ((this.getIndexOfElementID(idB): any): number), + (this.getIndexOfElementID(idA) || 0) - + (this.getIndexOfElementID(idB) || 0), ); // Next we need to determine the appropriate depth for each element in the list. @@ -677,7 +690,7 @@ export default class Store extends EventEmitter<{ // at which point, our depth is just the depth of that node plus one. sortedIDs.forEach(id => { const innerElement = this._idToElement.get(id); - if (innerElement != null) { + if (innerElement !== undefined) { let parentID = innerElement.parentID; let depth = 0; @@ -689,7 +702,7 @@ export default class Store extends EventEmitter<{ break; } const parent = this._idToElement.get(parentID); - if (parent == null) { + if (parent === undefined) { break; } parentID = parent.parentID; @@ -710,7 +723,7 @@ export default class Store extends EventEmitter<{ getRendererIDForElement(id: number): number | null { let current = this._idToElement.get(id); - while (current != null) { + while (current !== undefined) { if (current.parentID === 0) { const rendererID = this._rootIDToRendererID.get(current.id); return rendererID == null ? null : rendererID; @@ -723,7 +736,7 @@ export default class Store extends EventEmitter<{ getRootIDForElement(id: number): number | null { let current = this._idToElement.get(id); - while (current != null) { + while (current !== undefined) { if (current.parentID === 0) { return current.id; } else { @@ -765,10 +778,8 @@ export default class Store extends EventEmitter<{ const weightDelta = 1 - element.weight; - let parentElement: void | Element = ((this._idToElement.get( - element.parentID, - ): any): Element); - while (parentElement != null) { + let parentElement = this._idToElement.get(element.parentID); + while (parentElement !== undefined) { // We don't need to break on a collapsed parent in the same way as the expand case below. // That's because collapsing a node doesn't "bubble" and affect its parents. parentElement.weight += weightDelta; @@ -776,7 +787,7 @@ export default class Store extends EventEmitter<{ } } } else { - let currentElement = element; + let currentElement: ?Element = element; while (currentElement != null) { const oldWeight = currentElement.isCollapsed ? 1 @@ -791,10 +802,8 @@ export default class Store extends EventEmitter<{ : currentElement.weight; const weightDelta = newWeight - oldWeight; - let parentElement: void | Element = ((this._idToElement.get( - currentElement.parentID, - ): any): Element); - while (parentElement != null) { + let parentElement = this._idToElement.get(currentElement.parentID); + while (parentElement !== undefined) { parentElement.weight += weightDelta; if (parentElement.isCollapsed) { // It's important to break on a collapsed parent when expanding nodes. @@ -808,10 +817,8 @@ export default class Store extends EventEmitter<{ currentElement = currentElement.parentID !== 0 - ? // $FlowFixMe[incompatible-type] found when upgrading Flow - this.getElementByID(currentElement.parentID) - : // $FlowFixMe[incompatible-type] found when upgrading Flow - null; + ? this.getElementByID(currentElement.parentID) + : null; } } @@ -833,7 +840,7 @@ export default class Store extends EventEmitter<{ } _adjustParentTreeWeight: ( - parentElement: Element | null, + parentElement: ?Element, weightDelta: number, ) => void = (parentElement, weightDelta) => { let isInsideCollapsedSubTree = false; @@ -848,9 +855,7 @@ export default class Store extends EventEmitter<{ break; } - parentElement = ((this._idToElement.get( - parentElement.parentID, - ): any): Element); + parentElement = this._idToElement.get(parentElement.parentID); } // Additions and deletions within a collapsed subtree should not affect the overall number of elements. @@ -906,13 +911,16 @@ export default class Store extends EventEmitter<{ const stringTable: Array = [ null, // ID = 0 corresponds to the null string. ]; - const stringTableSize = operations[i++]; + const stringTableSize = operations[i]; + i++; + const stringTableEnd = i + stringTableSize; + while (i < stringTableEnd) { - const nextLength = operations[i++]; - const nextString = utfDecodeString( - (operations.slice(i, i + nextLength): any), - ); + const nextLength = operations[i]; + i++; + + const nextString = utfDecodeString(operations.slice(i, i + nextLength)); stringTable.push(nextString); i += nextLength; } @@ -921,7 +929,7 @@ export default class Store extends EventEmitter<{ const operation = operations[i]; switch (operation) { case TREE_OPERATION_ADD: { - const id = ((operations[i + 1]: any): number); + const id = operations[i + 1]; const type = ((operations[i + 2]: any): ElementType); i += 3; @@ -934,8 +942,6 @@ export default class Store extends EventEmitter<{ ); } - let ownerID: number = 0; - let parentID: number = ((null: any): number); if (type === ElementTypeRoot) { if (__DEBUG__) { debug('Add', `new root node ${id}`); @@ -997,10 +1003,10 @@ export default class Store extends EventEmitter<{ haveRootsChanged = true; } else { - parentID = ((operations[i]: any): number); + const parentID = operations[i]; i++; - ownerID = ((operations[i]: any): number); + const ownerID = operations[i]; i++; const displayNameStringID = operations[i]; @@ -1018,17 +1024,17 @@ export default class Store extends EventEmitter<{ ); } - if (!this._idToElement.has(parentID)) { + const parentElement = this._idToElement.get(parentID); + if (parentElement === undefined) { this._throwAndEmitError( Error( `Cannot add child "${id}" to parent "${parentID}" because parent node was not found in the Store.`, ), ); + + continue; } - const parentElement = ((this._idToElement.get( - parentID, - ): any): Element); parentElement.children.push(id); const [displayNameWithoutHOCs, hocDisplayNames] = @@ -1065,23 +1071,25 @@ export default class Store extends EventEmitter<{ break; } case TREE_OPERATION_REMOVE: { - const removeLength = ((operations[i + 1]: any): number); + const removeLength = operations[i + 1]; i += 2; for (let removeIndex = 0; removeIndex < removeLength; removeIndex++) { - const id = ((operations[i]: any): number); + const id = operations[i]; + const element = this._idToElement.get(id); - if (!this._idToElement.has(id)) { + if (element === undefined) { this._throwAndEmitError( Error( `Cannot remove node "${id}" because no matching node was found in the Store.`, ), ); + + continue; } i += 1; - const element = ((this._idToElement.get(id): any): Element); const {children, ownerID, parentID, weight} = element; if (children.length > 0) { this._throwAndEmitError( @@ -1091,7 +1099,7 @@ export default class Store extends EventEmitter<{ this._idToElement.delete(id); - let parentElement = null; + let parentElement: ?Element = null; if (parentID === 0) { if (__DEBUG__) { debug('Remove', `node ${id} root`); @@ -1106,14 +1114,18 @@ export default class Store extends EventEmitter<{ if (__DEBUG__) { debug('Remove', `node ${id} from parent ${parentID}`); } - parentElement = ((this._idToElement.get(parentID): any): Element); + + parentElement = this._idToElement.get(parentID); if (parentElement === undefined) { this._throwAndEmitError( Error( `Cannot remove node "${id}" from parent "${parentID}" because no matching node was found in the Store.`, ), ); + + continue; } + const index = parentElement.children.indexOf(id); parentElement.children.splice(index, 1); } @@ -1167,19 +1179,21 @@ export default class Store extends EventEmitter<{ break; } case TREE_OPERATION_REORDER_CHILDREN: { - const id = ((operations[i + 1]: any): number); - const numChildren = ((operations[i + 2]: any): number); + const id = operations[i + 1]; + const numChildren = operations[i + 2]; i += 3; - if (!this._idToElement.has(id)) { + const element = this._idToElement.get(id); + if (element === undefined) { this._throwAndEmitError( Error( `Cannot reorder children for node "${id}" because no matching node was found in the Store.`, ), ); + + continue; } - const element = ((this._idToElement.get(id): any): Element); const children = element.children; if (children.length !== numChildren) { this._throwAndEmitError(