Skip to content

DevTools: fix backend activation #26779

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 4, 2023
Merged

DevTools: fix backend activation #26779

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
04db661
fix[devtools]: activate already registered backends, remove them from…
hoxyq May 4, 2023
b7e43a0
refactor[devtools/store.js]: removed redundant types castings / non-f…
hoxyq May 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/react-devtools-extensions/src/backendManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 4 additions & 3 deletions packages/react-devtools-extensions/src/background.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 2 additions & 6 deletions packages/react-devtools-shared/src/backend/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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',
Expand Down Expand Up @@ -64,10 +65,6 @@ export function initBackend(
];

const attachRenderer = (id: number, renderer: ReactRenderer) => {
// skip if already attached
if (renderer.attached) {
return;
}
Comment on lines -67 to -70
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frontend side of devtools needs renderer-attached event to be emitted. With this check this will never happen and agent will not receive operations from hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @mondaychen

Are there any cases I am missing where we really need this check?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I think this was something I missed. Let's remove it now.

// only attach if the renderer is compatible with the current version of the backend
if (!isMatchingRender(renderer.reconcilerVersion || renderer.version)) {
return;
Expand Down Expand Up @@ -102,7 +99,6 @@ export function initBackend(
} else {
hook.emit('unsupported-renderer-version', id);
}
renderer.attached = true;
};

// Connect renderers that have already injected themselves.
Expand Down
2 changes: 0 additions & 2 deletions packages/react-devtools-shared/src/backend/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,6 @@ export type ReactRenderer = {
// 18.0+
injectProfilingHooks?: (profilingHooks: DevToolsProfilingHooks) => void,
getLaneLabelMap?: () => Map<Lane, string> | null,
// set by backend after successful attaching
attached?: boolean,
...
};

Expand Down
Loading