From c12194f7485f298fadc1e51cfffb93e63d61ad96 Mon Sep 17 00:00:00 2001 From: Mengdi Chen Date: Tue, 7 Feb 2023 07:59:44 -0500 Subject: [PATCH 1/9] [DevTools] improve error handling in extension (#26068) ## Summary This is to fix some edge cases I recently observed when developing and using the extension: - When you reload the page, there's a chance that a port (most likely the devtools one) is not properly unloaded. In this case, the React DevTools will stop working unless you create a new tab. - For unknown reasons, Chrome sometimes spins up two service worker processes. In this case, an error will be thrown "duplicate ID when registering content script" and sometimes interrupt the execution of the rest of service worker. This is an attempt to make the logic more robust - Automatically shutting down the double pipe if the message fails, and allowing the runtime to rebuild the double pipe. - Log the error message so Chrome believes we've handled it and will not interrupt the execution. This also seems to be helpful in fixing #25806. --- .../src/background.js | 63 +++++++++++++------ 1 file changed, 44 insertions(+), 19 deletions(-) diff --git a/packages/react-devtools-extensions/src/background.js b/packages/react-devtools-extensions/src/background.js index 70c2420b0df18..8ef96d2ec9b6f 100644 --- a/packages/react-devtools-extensions/src/background.js +++ b/packages/react-devtools-extensions/src/background.js @@ -12,22 +12,31 @@ if (!IS_FIREFOX) { // It's critical since it allows us to directly run scripts on the "main" world on the page // "document_start" allows it to run before the page's scripts // so the hook can be detected by react reconciler - chrome.scripting.registerContentScripts([ - { - id: 'hook', - matches: [''], - js: ['build/installHook.js'], - runAt: 'document_start', - world: chrome.scripting.ExecutionWorld.MAIN, - }, - { - id: 'renderer', - matches: [''], - js: ['build/renderer.js'], - runAt: 'document_start', - world: chrome.scripting.ExecutionWorld.MAIN, + chrome.scripting.registerContentScripts( + [ + { + id: 'hook', + matches: [''], + js: ['build/installHook.js'], + runAt: 'document_start', + world: chrome.scripting.ExecutionWorld.MAIN, + }, + { + id: 'renderer', + matches: [''], + 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. + // It happens when the service worker process is incorrectly duplicated. + if (chrome.runtime.lastError) { + console.error(chrome.runtime.lastError); + } }, - ]); + ); } chrome.runtime.onConnect.addListener(function (port) { @@ -51,7 +60,7 @@ chrome.runtime.onConnect.addListener(function (port) { ports[tab][name] = port; if (ports[tab].devtools && ports[tab]['content-script']) { - doublePipe(ports[tab].devtools, ports[tab]['content-script']); + doublePipe(ports[tab].devtools, ports[tab]['content-script'], tab); } }); @@ -70,20 +79,36 @@ function installProxy(tabId: number) { } } -function doublePipe(one, two) { +function doublePipe(one, two, tabId) { one.onMessage.addListener(lOne); function lOne(message) { - two.postMessage(message); + try { + two.postMessage(message); + } catch (e) { + if (__DEV__) { + console.log(`Broken pipe ${tabId}: `, e); + } + shutdown(); + } } two.onMessage.addListener(lTwo); function lTwo(message) { - one.postMessage(message); + try { + one.postMessage(message); + } catch (e) { + if (__DEV__) { + console.log(`Broken pipe ${tabId}: `, e); + } + shutdown(); + } } function shutdown() { one.onMessage.removeListener(lOne); two.onMessage.removeListener(lTwo); one.disconnect(); two.disconnect(); + // clean up so that we can rebuild the double pipe if the page is reloaded + ports[tabId] = null; } one.onDisconnect.addListener(shutdown); two.onDisconnect.addListener(shutdown); From 13f4ccfdba06b599a5db7c5a192024cbfa365edc Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Tue, 7 Feb 2023 17:57:43 +0100 Subject: [PATCH 2/9] Fix main (#26120) ## Summary Prettier was bumped recently. So any branch not including that bump, might bring in outdated formatting (e.g. https://github.com/facebook/react/pull/26068) ## How did you test this change? - [x] `yarn prettier-all` --- packages/react-devtools-extensions/src/background.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-devtools-extensions/src/background.js b/packages/react-devtools-extensions/src/background.js index 8ef96d2ec9b6f..c2203146862b1 100644 --- a/packages/react-devtools-extensions/src/background.js +++ b/packages/react-devtools-extensions/src/background.js @@ -29,7 +29,7 @@ if (!IS_FIREFOX) { world: chrome.scripting.ExecutionWorld.MAIN, }, ], - function() { + function () { // When the content scripts are already registered, an error will be thrown. // It happens when the service worker process is incorrectly duplicated. if (chrome.runtime.lastError) { From f0cf832e1d0c8544c36aa8b310960885a11a847c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 7 Feb 2023 12:09:29 -0500 Subject: [PATCH 3/9] Update Flight Fixture to "use client" instead of .client.js (#26118) This updates the Flight fixture to support the new ESM loaders in newer versions of Node.js. It also uses native fetch since react-fetch is gone now. (This part requires Node 18 to run the fixture.) I also updated everything to use the `"use client"` convention instead of file name based convention. The biggest hack here is that the Webpack plugin now just writes every `.js` file in the manifest. This needs to be more scoped. In practice, this new convention effectively requires you to traverse the server graph first to find the actual used files. This is enough to at least run our own fixture though. I didn't update the "blocks" fixture. More details in each commit message. --- fixtures/flight-browser/index.html | 8 +- fixtures/flight/config/webpack.config.js | 9 +- fixtures/flight/loader/index.js | 30 ++- .../flight/server/{cli.server.js => cli.js} | 2 +- .../server/{handler.server.js => handler.js} | 4 +- fixtures/flight/server/package.json | 2 +- fixtures/flight/src/{App.server.js => App.js} | 12 +- .../src/{Counter.client.js => Counter.js} | 2 + fixtures/flight/src/Counter2.client.js | 1 - fixtures/flight/src/Counter2.js | 3 + .../src/{ShowMore.client.js => ShowMore.js} | 2 + .../src/ReactFlightWebpackNodeLoader.js | 233 +++++++++++------- .../src/ReactFlightWebpackNodeRegister.js | 71 +++--- .../src/ReactFlightWebpackPlugin.js | 4 +- .../src/__tests__/utils/WebpackMock.js | 16 +- scripts/rollup/bundles.js | 19 +- 16 files changed, 247 insertions(+), 171 deletions(-) rename fixtures/flight/server/{cli.server.js => cli.js} (97%) rename fixtures/flight/server/{handler.server.js => handler.js} (89%) rename fixtures/flight/src/{App.server.js => App.js} (56%) rename fixtures/flight/src/{Counter.client.js => Counter.js} (94%) delete mode 100644 fixtures/flight/src/Counter2.client.js create mode 100644 fixtures/flight/src/Counter2.js rename fixtures/flight/src/{ShowMore.client.js => ShowMore.js} (95%) diff --git a/fixtures/flight-browser/index.html b/fixtures/flight-browser/index.html index 927a0556d5328..5b16a2809c7a1 100644 --- a/fixtures/flight-browser/index.html +++ b/fixtures/flight-browser/index.html @@ -20,7 +20,7 @@

Flight Example

- +