Skip to content
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.

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

[DevTools] remove backend dependency from the global hook #26563

Merged
merged 4 commits into from
Apr 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions packages/react-devtools-core/src/backend.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import Agent from 'react-devtools-shared/src/backend/agent';
import Bridge from 'react-devtools-shared/src/bridge';
import {installHook} from 'react-devtools-shared/src/hook';
import {initBackend} from 'react-devtools-shared/src/backend';
import {installConsoleFunctionsToWindow} from 'react-devtools-shared/src/backend/console';
import {__DEBUG__} from 'react-devtools-shared/src/constants';
import setupNativeStyleEditor from 'react-devtools-shared/src/backend/NativeStyleEditor/setupNativeStyleEditor';
import {getDefaultComponentFilters} from 'react-devtools-shared/src/utils';
Expand Down Expand Up @@ -38,6 +39,9 @@ type ConnectOptions = {
...
};

// Install a global variable to allow patching console early (during injection).
// This provides React Native developers with components stacks even if they don't run DevTools.
installConsoleFunctionsToWindow();
installHook(window);

const hook: ?DevToolsHook = window.__REACT_DEVTOOLS_GLOBAL_HOOK__;
Expand Down
1 change: 1 addition & 0 deletions packages/react-devtools-extensions/chrome/manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
"panel.html",
"build/react_devtools_backend.js",
"build/proxy.js",
"build/renderer.js",
"build/installHook.js"
],
"matches": [
Expand Down
1 change: 1 addition & 0 deletions packages/react-devtools-extensions/edge/manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
"panel.html",
"build/react_devtools_backend.js",
"build/proxy.js",
"build/renderer.js",
"build/installHook.js"
],
"matches": [
Expand Down
1 change: 1 addition & 0 deletions packages/react-devtools-extensions/firefox/manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
"panel.html",
"build/react_devtools_backend.js",
"build/proxy.js",
"build/renderer.js",
"build/installHook.js"
],
"background": {
Expand Down
7 changes: 7 additions & 0 deletions packages/react-devtools-extensions/src/background.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@ if (!IS_FIREFOX) {
runAt: 'document_start',
world: chrome.scripting.ExecutionWorld.MAIN,
},
{
id: 'renderer',
matches: ['<all_urls>'],
js: ['build/renderer.js'],
runAt: 'document_start',
world: chrome.scripting.ExecutionWorld.MAIN,
},
],
function () {
// When the content scripts are already registered, an error will be thrown.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
/* global chrome */

import nullthrows from 'nullthrows';
import {SESSION_STORAGE_RELOAD_AND_PROFILE_KEY} from 'react-devtools-shared/src/constants';
import {sessionStorageGetItem} from 'react-devtools-shared/src/storage';
import {IS_FIREFOX} from '../utils';

// We run scripts on the page via the service worker (backgroud.js) for
Expand Down Expand Up @@ -109,9 +111,15 @@ window.addEventListener('pageshow', function ({target}) {
chrome.runtime.sendMessage(lastDetectionResult);
});

// Inject a __REACT_DEVTOOLS_GLOBAL_HOOK__ global for React to interact with.
// Only do this for HTML documents though, to avoid e.g. breaking syntax highlighting for XML docs.
if (IS_FIREFOX) {
// If we have just reloaded to profile, we need to inject the renderer interface before the app loads.
if (
sessionStorageGetItem(SESSION_STORAGE_RELOAD_AND_PROFILE_KEY) === 'true'
) {
injectScriptSync(chrome.runtime.getURL('build/renderer.js'));
}
// Inject a __REACT_DEVTOOLS_GLOBAL_HOOK__ global for React to interact with.
// Only do this for HTML documents though, to avoid e.g. breaking syntax highlighting for XML docs.
switch (document.contentType) {
case 'text/html':
case 'application/xhtml+xml': {
Expand Down
33 changes: 33 additions & 0 deletions packages/react-devtools-extensions/src/contentScripts/renderer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* In order to support reload-and-profile functionality, the renderer needs to be injected before any other scripts.
* Since it is a complex file (with imports) we can't just toString() it like we do with the hook itself,
* So this entry point (one of the web_accessible_resources) provides a way to eagerly inject it.
* The hook will look for the presence of a global __REACT_DEVTOOLS_ATTACH__ and attach an injected renderer early.
* The normal case (not a reload-and-profile) will not make use of this entry point though.
*
* @flow
*/

import {attach} from 'react-devtools-shared/src/backend/renderer';
import {SESSION_STORAGE_RELOAD_AND_PROFILE_KEY} from 'react-devtools-shared/src/constants';
import {sessionStorageGetItem} from 'react-devtools-shared/src/storage';

if (
sessionStorageGetItem(SESSION_STORAGE_RELOAD_AND_PROFILE_KEY) === 'true' &&
!window.hasOwnProperty('__REACT_DEVTOOLS_ATTACH__')
) {
Object.defineProperty(
window,
'__REACT_DEVTOOLS_ATTACH__',
({
enumerable: false,
// This property needs to be configurable to allow third-party integrations
// to attach their own renderer. Note that using third-party integrations
// is not officially supported. Use at your own risk.
configurable: true,
get() {
return attach;
},
}: Object),
);
}
1 change: 1 addition & 0 deletions packages/react-devtools-extensions/webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ module.exports = {
panel: './src/panel.js',
proxy: './src/contentScripts/proxy.js',
prepareInjection: './src/contentScripts/prepareInjection.js',
renderer: './src/contentScripts/renderer.js',
installHook: './src/contentScripts/installHook.js',
},
output: {
Expand Down
4 changes: 4 additions & 0 deletions packages/react-devtools-inline/src/backend.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import Agent from 'react-devtools-shared/src/backend/agent';
import Bridge from 'react-devtools-shared/src/bridge';
import {initBackend} from 'react-devtools-shared/src/backend';
import {installConsoleFunctionsToWindow} from 'react-devtools-shared/src/backend/console';
import {installHook} from 'react-devtools-shared/src/hook';
import setupNativeStyleEditor from 'react-devtools-shared/src/backend/NativeStyleEditor/setupNativeStyleEditor';

Expand Down Expand Up @@ -119,5 +120,8 @@ export function createBridge(contentWindow: any, wall?: Wall): BackendBridge {
}

export function initialize(contentWindow: any): void {
// Install a global variable to allow patching console early (during injection).
// This provides React Native developers with components stacks even if they don't run DevTools.
installConsoleFunctionsToWindow();
installHook(contentWindow);
}
7 changes: 7 additions & 0 deletions packages/react-devtools-shared/src/backend/console.js
Original file line number Diff line number Diff line change
Expand Up @@ -413,3 +413,10 @@ export function writeConsolePatchSettingsToWindow(
settings.hideConsoleLogsInStrictMode;
window.__REACT_DEVTOOLS_BROWSER_THEME__ = settings.browserTheme;
}

export function installConsoleFunctionsToWindow(): void {
window.__REACT_DEVTOOLS_CONSOLE_FUNCTIONS__ = {
patchConsoleUsingWindowValues,
registerRendererWithConsole: registerRenderer,
};
}
3 changes: 1 addition & 2 deletions packages/react-devtools-shared/src/backend/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,7 @@ export function initBackend(
}

// Notify the DevTools frontend about new renderers.
// This includes any that were attached early
// (when SESSION_STORAGE_RELOAD_AND_PROFILE_KEY is set to true).
// This includes any that were attached early (via __REACT_DEVTOOLS_ATTACH__).
if (rendererInterface != null) {
hook.emit('renderer-attached', {
id,
Expand Down
51 changes: 15 additions & 36 deletions packages/react-devtools-shared/src/hook.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
/**
* Install the hook on window, which is an event emitter.
* Note because Chrome content scripts cannot directly modify the window object,
* we are evaling this function by inserting a script tag.
* That's why we have to inline the whole event emitter implementation,
* Note: this global hook __REACT_DEVTOOLS_GLOBAL_HOOK__ is a de facto public API.
* It's especially important to avoid creating direct dependency on the DevTools Backend.
* That's why we still inline the whole event emitter implementation,
* the string format implementation, and part of the console implementation here.
*
* @flow
Expand All @@ -17,14 +17,6 @@ import type {
RendererInterface,
} from './backend/types';

import {
patchConsoleUsingWindowValues,
registerRenderer as registerRendererWithConsole,
} from './backend/console';
import {attach} from './backend/renderer';
import {SESSION_STORAGE_RELOAD_AND_PROFILE_KEY} from './constants';
import {sessionStorageGetItem} from './storage';

declare var window: any;

export function installHook(target: any): DevToolsHook | null {
Expand Down Expand Up @@ -340,37 +332,24 @@ export function installHook(target: any): DevToolsHook | null {
// * Disabling or marking logs during a double render in Strict Mode
// * Disable logging during re-renders to inspect hooks (see inspectHooksOfFiber)
//
// For React Native, we intentionally patch early (during injection).
// This provides React Native developers with components stacks even if they don't run DevTools.
//
// This won't work for DOM though, since this entire file is eval'ed and inserted as a script tag.
// In that case, we'll only patch parts of the console that are needed during the first render
// and patch everything else later (when the frontend attaches).
//
// Don't patch in test environments because we don't want to interfere with Jest's own console overrides.
//
// Note that because this function is inlined, this conditional check must only use static booleans.
// Otherwise the extension will throw with an undefined error.
// (See comments in the try/catch below for more context on inlining.)
if (!__TEST__ && !__EXTENSION__) {
try {
// The installHook() function is injected by being stringified in the browser,
// so imports outside of this function do not get included.
//
// Normally we could check "typeof patchConsole === 'function'",
// but Webpack wraps imports with an object (e.g. _backend_console__WEBPACK_IMPORTED_MODULE_0__)
// and the object itself will be undefined as well for the reasons mentioned above,
// so we use try/catch instead.
// Allow patching console early (during injection) to
// provide developers with components stacks even if they don't run DevTools.
if (target.hasOwnProperty('__REACT_DEVTOOLS_CONSOLE_FUNCTIONS__')) {
const {registerRendererWithConsole, patchConsoleUsingWindowValues} =
target.__REACT_DEVTOOLS_CONSOLE_FUNCTIONS__;
if (
typeof registerRendererWithConsole === 'function' &&
typeof patchConsoleUsingWindowValues === 'function'
) {
registerRendererWithConsole(renderer);
patchConsoleUsingWindowValues();
} catch (error) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

What could error before and why are we removing the catch?

Copy link
Contributor Author

@mondaychen mondaychen Apr 7, 2023

Choose a reason for hiding this comment

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

That's a great question! In the original implementation, the hook is injected into the page via an inline script tag with installHook.toString() as the content. For example:

injectCode(
';(' +
installHook.toString() +
'(window))' +
saveNativeValues +
detectReact,
);

Because webpack will add some prefix (like _backend_console__WEBPACK_IMPORTED_MODULE_0__) to the modules imported in hook.js during compiling, it would actually always cause an error in the extension. This try-catch is for that purpose.
The comments on the top of hook.js says "we have to inline the whole ... implementation here" for the same reason.
Now we are using a much more elegant way to inject the hook, this is no longer necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

People probably wanted to patch as early as possible for all platforms. But they didn't find a way to make it work for the extension. Then they noticed that the DevTools backend is initialized automatically by the extension, so it was accepted to patch it a bit later.

Copy link
Contributor Author

@mondaychen mondaychen Apr 7, 2023

Choose a reason for hiding this comment

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

This patched console depends on WorkTagMap, which is defined in the reconciler and can be changed in different versions. Going forward I think it's the best to keep it with the devtools backend. On dev and profiling builds we will be able to patch it early even on the web. On prod builds it should be OK to not have this additional information before the DevTools backend is initialized.

I'd like to hear thoughts from @gaearon and @acdlite, if any

}
}

// If we have just reloaded to profile, we need to inject the renderer interface before the app loads.
// Otherwise the renderer won't yet exist and we can skip this step.
if (
sessionStorageGetItem(SESSION_STORAGE_RELOAD_AND_PROFILE_KEY) === 'true'
) {
const attach = target.__REACT_DEVTOOLS_ATTACH__;
if (typeof attach === 'function') {
const rendererInterface = attach(hook, id, renderer, target);
hook.rendererInterfaces.set(id, rendererInterface);
}
Expand Down