From 943ec4045ad780862dfae9ff476c0c0aec5ecbf9 Mon Sep 17 00:00:00 2001 From: Alex Hunt Date: Fri, 24 May 2024 08:00:08 -0700 Subject: [PATCH 1/3] Revert debugger launch behaviour to default profile (#44638) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Reverts the debugger launch flow to use the default `ChromeLauncher` profile. This is the approach used in the current `--experimental-debugger` experiment and by Expo. This is motivated after a review of the tradeoffs of a guest profile — which allow us to programatically quit the browser process, however takes over system URL handling. Changelog: [Internal] Differential Revision: D57619542 --- packages/dev-middleware/src/utils/DefaultBrowserLauncher.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/dev-middleware/src/utils/DefaultBrowserLauncher.js b/packages/dev-middleware/src/utils/DefaultBrowserLauncher.js index 9f4153fa3081..705d0c4b6d64 100644 --- a/packages/dev-middleware/src/utils/DefaultBrowserLauncher.js +++ b/packages/dev-middleware/src/utils/DefaultBrowserLauncher.js @@ -51,6 +51,7 @@ const DefaultBrowserLauncher: BrowserLauncher = { `react-native-debugger-frontend-${browserType}`, ); const launchedChrome = await ChromeLauncher.launch({ + ignoreDefaultFlags: true, chromeFlags: [ ...ChromeLauncher.Launcher.defaultFlags().filter( /** @@ -64,10 +65,8 @@ const DefaultBrowserLauncher: BrowserLauncher = { `--app=${url}`, `--user-data-dir=${userDataDir}`, '--window-size=1200,600', - '--guest', ], chromePath, - ignoreDefaultFlags: true, }); return { From 8f0d20c94f3489a3f03135b0ddd30227ad7234f2 Mon Sep 17 00:00:00 2001 From: Alex Hunt Date: Fri, 24 May 2024 08:00:08 -0700 Subject: [PATCH 2/3] Simplify debugger launch flow to invoke Chrome directly, drop kill API Summary: Swaps out and simplifies the internals of the debugger launch flow. We observed that we could achieve better launch/windowing behaviour by passing the `--app` argument directly to the detected Chrome path. This shares the user's default Chrome profile: - Fixes unwanted behaviour such as a separate dock icon on macOS (which, when clicked, would launch an unwanted empty window). - Enables settings persistence. This change also removes the `LaunchedBrowser.kill` API. Changelog: [Internal] Differential Revision: D57726649 --- flow-typed/npm/temp-dir_2.x.x.js | 14 ----- packages/dev-middleware/package.json | 1 - packages/dev-middleware/src/index.flow.js | 2 +- .../src/middleware/openDebuggerMiddleware.js | 24 +++---- .../src/types/BrowserLauncher.js | 10 +-- .../src/utils/DefaultBrowserLauncher.js | 63 +++++++------------ yarn.lock | 5 -- 7 files changed, 32 insertions(+), 87 deletions(-) delete mode 100644 flow-typed/npm/temp-dir_2.x.x.js diff --git a/flow-typed/npm/temp-dir_2.x.x.js b/flow-typed/npm/temp-dir_2.x.x.js deleted file mode 100644 index 23b41d09bb8a..000000000000 --- a/flow-typed/npm/temp-dir_2.x.x.js +++ /dev/null @@ -1,14 +0,0 @@ -/** - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - * - * @flow strict - * @format - * @oncall react_native - */ - -declare module 'temp-dir' { - declare module.exports: string; -} diff --git a/packages/dev-middleware/package.json b/packages/dev-middleware/package.json index 0b174b0c1daa..a1313b464208 100644 --- a/packages/dev-middleware/package.json +++ b/packages/dev-middleware/package.json @@ -33,7 +33,6 @@ "open": "^7.0.3", "selfsigned": "^2.4.1", "serve-static": "^1.13.1", - "temp-dir": "^2.0.0", "ws": "^6.2.2" }, "engines": { diff --git a/packages/dev-middleware/src/index.flow.js b/packages/dev-middleware/src/index.flow.js index 7afbbdbd8894..6bd13afb5f84 100644 --- a/packages/dev-middleware/src/index.flow.js +++ b/packages/dev-middleware/src/index.flow.js @@ -11,7 +11,7 @@ export {default as createDevMiddleware} from './createDevMiddleware'; -export type {BrowserLauncher, LaunchedBrowser} from './types/BrowserLauncher'; +export type {BrowserLauncher} from './types/BrowserLauncher'; export type {EventReporter, ReportableEvent} from './types/EventReporter'; export type { CustomMessageHandler, diff --git a/packages/dev-middleware/src/middleware/openDebuggerMiddleware.js b/packages/dev-middleware/src/middleware/openDebuggerMiddleware.js index 4099a7bfadfc..56786060ad7d 100644 --- a/packages/dev-middleware/src/middleware/openDebuggerMiddleware.js +++ b/packages/dev-middleware/src/middleware/openDebuggerMiddleware.js @@ -10,7 +10,7 @@ */ import type {InspectorProxyQueries} from '../inspector-proxy/InspectorProxy'; -import type {BrowserLauncher, LaunchedBrowser} from '../types/BrowserLauncher'; +import type {BrowserLauncher} from '../types/BrowserLauncher'; import type {EventReporter} from '../types/EventReporter'; import type {Experiments} from '../types/Experiments'; import type {Logger} from '../types/Logger'; @@ -20,8 +20,6 @@ import type {IncomingMessage, ServerResponse} from 'http'; import getDevToolsFrontendUrl from '../utils/getDevToolsFrontendUrl'; import url from 'url'; -const debuggerInstances = new Map(); - type Options = $ReadOnly<{ serverBaseUrl: string, logger?: Logger, @@ -117,20 +115,12 @@ export default function openDebuggerMiddleware({ try { switch (launchType) { case 'launch': - const frontendInstanceId = - device != null - ? 'device:' + device - : 'app:' + (appId ?? ''); - await debuggerInstances.get(frontendInstanceId)?.kill(); - debuggerInstances.set( - frontendInstanceId, - await browserLauncher.launchDebuggerAppWindow( - getDevToolsFrontendUrl( - experiments, - target.webSocketDebuggerUrl, - serverBaseUrl, - {launchId, useFuseboxEntryPoint}, - ), + await browserLauncher.launchDebuggerAppWindow( + getDevToolsFrontendUrl( + experiments, + target.webSocketDebuggerUrl, + serverBaseUrl, + {launchId, useFuseboxEntryPoint}, ), ); res.end(); diff --git a/packages/dev-middleware/src/types/BrowserLauncher.js b/packages/dev-middleware/src/types/BrowserLauncher.js index d6a5b91d97fc..d76b31ec5965 100644 --- a/packages/dev-middleware/src/types/BrowserLauncher.js +++ b/packages/dev-middleware/src/types/BrowserLauncher.js @@ -9,14 +9,6 @@ * @oncall react_native */ -/** - * Represents a launched web browser instance. - */ -export type LaunchedBrowser = { - kill: () => void | Promise, - ... -}; - /** * An interface for integrators to provide a custom implementation for * opening URLs in a web browser. @@ -27,5 +19,5 @@ export interface BrowserLauncher { * optionally returning an object to control the launched browser instance. * The browser used should be capable of running Chrome DevTools. */ - launchDebuggerAppWindow: (url: string) => Promise; + launchDebuggerAppWindow: (url: string) => Promise; } diff --git a/packages/dev-middleware/src/utils/DefaultBrowserLauncher.js b/packages/dev-middleware/src/utils/DefaultBrowserLauncher.js index 705d0c4b6d64..94c745de3ffb 100644 --- a/packages/dev-middleware/src/utils/DefaultBrowserLauncher.js +++ b/packages/dev-middleware/src/utils/DefaultBrowserLauncher.js @@ -9,12 +9,9 @@ * @oncall react_native */ -import type {BrowserLauncher, LaunchedBrowser} from '../types/BrowserLauncher'; - -import {promises as fs} from 'fs'; -import path from 'path'; -import osTempDir from 'temp-dir'; +import type {BrowserLauncher} from '../types/BrowserLauncher'; +const {spawn} = require('child_process'); const ChromeLauncher = require('chrome-launcher'); const {Launcher: EdgeLauncher} = require('chromium-edge-launcher'); @@ -27,15 +24,13 @@ const DefaultBrowserLauncher: BrowserLauncher = { * Attempt to open the debugger frontend in a Google Chrome or Microsoft Edge * app window. */ - launchDebuggerAppWindow: async (url: string): Promise => { - let browserType = 'chrome'; + launchDebuggerAppWindow: async url => { let chromePath; try { // Locate Chrome installation path, will throw if not found chromePath = ChromeLauncher.getChromePath(); } catch (e) { - browserType = 'edge'; chromePath = EdgeLauncher.getFirstInstallation(); if (chromePath == null) { @@ -47,40 +42,28 @@ const DefaultBrowserLauncher: BrowserLauncher = { } } - const userDataDir = await createTempDir( - `react-native-debugger-frontend-${browserType}`, - ); - const launchedChrome = await ChromeLauncher.launch({ - ignoreDefaultFlags: true, - chromeFlags: [ - ...ChromeLauncher.Launcher.defaultFlags().filter( - /** - * This flag controls whether Chrome treats a visually covered (occluded) tab - * as "backgrounded". We launch CDT as a single tab/window via `--app`, so we - * do want Chrome to treat our tab as "backgrounded" when the UI is covered. - * Omitting this flag allows "visibilitychange" events to fire properly. - */ - flag => flag !== '--disable-backgrounding-occluded-windows', - ), - `--app=${url}`, - `--user-data-dir=${userDataDir}`, - '--window-size=1200,600', - ], - chromePath, - }); + const chromeFlags = [`--app=${url}`, '--window-size=1200,600']; - return { - kill: async () => launchedChrome.kill(), - }; + return new Promise((resolve, reject) => { + const childProcess = spawn(chromePath, chromeFlags, { + detached: true, + stdio: 'ignore', + }); + + childProcess.on('data', () => { + resolve(); + }); + childProcess.on('close', (code: number) => { + if (code !== 0) { + reject( + new Error( + `Failed to launch debugger app window: ${chromePath} exited with code ${code}`, + ), + ); + } + }); + }); }, }; -async function createTempDir(dirName: string): Promise { - const tempDir = path.join(osTempDir, dirName); - - await fs.mkdir(tempDir, {recursive: true}); - - return tempDir; -} - export default DefaultBrowserLauncher; diff --git a/yarn.lock b/yarn.lock index 59d808473e2a..0c994902b630 100644 --- a/yarn.lock +++ b/yarn.lock @@ -9567,11 +9567,6 @@ tar@^6.1.11: mkdirp "^1.0.3" yallist "^4.0.0" -temp-dir@^2.0.0: - version "2.0.0" - resolved "https://registry.yarnpkg.com/temp-dir/-/temp-dir-2.0.0.tgz#bde92b05bdfeb1516e804c9c00ad45177f31321e" - integrity sha512-aoBAniQmmwtcKp/7BzsH8Cxzv8OL736p7v1ihGb5e9DJ9kTwGWHrQrVB5+lfVDzfGrdRzXch+ig7LHaY1JTOrg== - temp@^0.8.4: version "0.8.4" resolved "https://registry.yarnpkg.com/temp/-/temp-0.8.4.tgz#8c97a33a4770072e0a05f919396c7665a7dd59f2" From c730c3d365581146437480b06bef88970f7d873d Mon Sep 17 00:00:00 2001 From: Alex Hunt Date: Fri, 24 May 2024 08:00:08 -0700 Subject: [PATCH 3/3] Add default browser fallback to debugger launch flow Summary: Changelog: [Internal] Differential Revision: D57726648 --- .../src/utils/DefaultBrowserLauncher.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/packages/dev-middleware/src/utils/DefaultBrowserLauncher.js b/packages/dev-middleware/src/utils/DefaultBrowserLauncher.js index 94c745de3ffb..67e5b899c2c1 100644 --- a/packages/dev-middleware/src/utils/DefaultBrowserLauncher.js +++ b/packages/dev-middleware/src/utils/DefaultBrowserLauncher.js @@ -14,6 +14,7 @@ import type {BrowserLauncher} from '../types/BrowserLauncher'; const {spawn} = require('child_process'); const ChromeLauncher = require('chrome-launcher'); const {Launcher: EdgeLauncher} = require('chromium-edge-launcher'); +const open = require('open'); /** * Default `BrowserLauncher` implementation which opens URLs on the host @@ -31,15 +32,15 @@ const DefaultBrowserLauncher: BrowserLauncher = { // Locate Chrome installation path, will throw if not found chromePath = ChromeLauncher.getChromePath(); } catch (e) { + // Fall back to Microsoft Edge chromePath = EdgeLauncher.getFirstInstallation(); + } - if (chromePath == null) { - throw new Error( - 'Unable to find a browser on the host to open the debugger. ' + - 'Supported browsers: Google Chrome, Microsoft Edge.\n' + - url, - ); - } + if (chromePath == null) { + // Fall back to default browser - the frontend will warn if the browser + // is not supported. + await open(url); + return; } const chromeFlags = [`--app=${url}`, '--window-size=1200,600'];