From c6a94bc170cf95a6bb21b5638929ec3311a9a5b7 Mon Sep 17 00:00:00 2001 From: Cedric van Putten Date: Thu, 8 Jun 2023 02:21:02 -0700 Subject: [PATCH] Allow client-side device identifiers in inspector proxy (#991) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Fixes https://github.com/facebook/metro/issues/985 This allows devices connecting to the inspector proxy to provide a client-side unique string identifier. With this, we can pass along a unique identifier that doesn't change when reconnecting, e.g. after a hard crash. It makes the overall debugging experience way more stable since you don't have to restart the debugger in between crashes or full restarts. Allowing users to keep the debugger open also lets the debugger "remember" certain things, such as set breakpoints. > ⚠️ When the client doesn't specify this unique identifier, it still falls back to the incremental identifier (the `fallbackDeviceId` in this PR) When a collision occurs, the old device's connection is closed. But, if both the device and app names are equal to the new device connection, the debugger connection is kept open. ## Next steps If this is landed, we still need changes in React Native (the `&device=...` query param). I have no strong opinions on what identifier is used, but it should follow these rules: - Should be unique per device or emulator/simulator (and thus, also per platform) - Must not change when restarting the app - Must be a string that's URL safe On Android, I had good success using the [`Secure.ANDROID_ID`](https://developer.android.com/reference/android/provider/Settings.Secure#ANDROID_ID). It may change after a factory reset, which doesn't matter for our use case. Pull Request resolved: https://github.com/facebook/metro/pull/991 Test Plan: - Create a new project, and enable building from source on both Android & iOS - **android**: edit [this file](https://github.com/facebook/react-native/blob/main/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevServerHelper.java#L282-L289) in `node_modules/react-native` and add a hardcoded `&device=testingandroid` query param. - **ios**: edit [this file](https://github.com/facebook/react-native/blob/main/packages/react-native/React/DevSupport/RCTInspectorDevServerHelper.mm#L43-L53) in `node_modules/react-naive` and add a hardcoded `&device=testingios` query param. - Connect the debugger to the running app - Force close the app, which should cause a "reconnect" warning in the debugger - Open the app again, and press "reconnect" in the debugger - _Due to the stable identifiers, the URL won't change and the above scenario should work fine_ Also test without these `&device=...` query param since that should work as it does now (with incremental identifiers). Reviewed By: motiz88 Differential Revision: D46482492 Pulled By: huntie fbshipit-source-id: 5d6e1e9c692d78ebfde8f4938041a8f381a1d48c --- packages/metro-inspector-proxy/src/Device.js | 39 ++++++++++++++++++- .../src/InspectorProxy.js | 29 ++++++++++---- 2 files changed, 59 insertions(+), 9 deletions(-) diff --git a/packages/metro-inspector-proxy/src/Device.js b/packages/metro-inspector-proxy/src/Device.js index cffa4f345d..a15856cdfe 100644 --- a/packages/metro-inspector-proxy/src/Device.js +++ b/packages/metro-inspector-proxy/src/Device.js @@ -55,7 +55,7 @@ const REACT_NATIVE_RELOADABLE_PAGE_ID = '-1'; */ class Device { // ID of the device. - _id: number; + _id: string; // Name of the device. _name: string; @@ -90,7 +90,7 @@ class Device { _projectRoot: string; constructor( - id: number, + id: string, name: string, app: string, socket: typeof WS, @@ -134,6 +134,10 @@ class Device { return this._name; } + getApp(): string { + return this._app; + } + getPagesList(): Array { if (this._lastConnectedReactNativePage) { const reactNativeReloadablePage = { @@ -212,6 +216,37 @@ class Device { }; } + /** + * Handles cleaning up a duplicate device connection, by client-side device ID. + * 1. Checks if the same device is attempting to reconnect for the same app. + * 2. If not, close both the device and debugger socket. + * 3. If the debugger connection can be reused, close the device socket only. + * + * This allows users to reload the app, either as result of a crash, or manually + * reloading, without having to restart the debugger. + */ + handleDuplicateDeviceConnection(newDevice: Device) { + if ( + this._app !== newDevice.getApp() || + this._name !== newDevice.getName() + ) { + this._deviceSocket.close(); + this._debuggerConnection?.socket.close(); + } + + const oldDebugger = this._debuggerConnection; + this._debuggerConnection = null; + + if (oldDebugger) { + oldDebugger.socket.removeAllListeners(); + this._deviceSocket.close(); + newDevice.handleDebuggerConnection( + oldDebugger.socket, + oldDebugger.pageId, + ); + } + } + // Handles messages received from device: // 1. For getPages responses updates local _pages list. // 2. All other messages are forwarded to debugger as wrappedEvent. diff --git a/packages/metro-inspector-proxy/src/InspectorProxy.js b/packages/metro-inspector-proxy/src/InspectorProxy.js index 96d4f0b19c..d9ee0ce18e 100644 --- a/packages/metro-inspector-proxy/src/InspectorProxy.js +++ b/packages/metro-inspector-proxy/src/InspectorProxy.js @@ -41,7 +41,7 @@ class InspectorProxy { _projectRoot: string; // Maps device ID to Device instance. - _devices: Map; + _devices: Map; // Internal counter for device IDs -- just gets incremented for each new device. _deviceCounter: number = 0; @@ -111,7 +111,7 @@ class InspectorProxy { // Converts page information received from device into PageDescription object // that is sent to debugger. _buildPageDescription( - deviceId: number, + deviceId: string, device: Device, page: Page, ): PageDescription { @@ -162,16 +162,31 @@ class InspectorProxy { // $FlowFixMe[value-as-type] wss.on('connection', async (socket: WS, req) => { try { + const fallbackDeviceId = String(this._deviceCounter++); + const query = url.parse(req.url || '', true).query || {}; + const deviceId = query.device || fallbackDeviceId; const deviceName = query.name || 'Unknown'; const appName = query.app || 'Unknown'; - const deviceId = this._deviceCounter++; - this._devices.set( + + const oldDevice = this._devices.get(deviceId); + const newDevice = new Device( deviceId, - new Device(deviceId, deviceName, appName, socket, this._projectRoot), + deviceName, + appName, + socket, + this._projectRoot, ); - debug(`Got new connection: device=${deviceName}, app=${appName}`); + if (oldDevice) { + oldDevice.handleDuplicateDeviceConnection(newDevice); + } + + this._devices.set(deviceId, newDevice); + + debug( + `Got new connection: name=${deviceName}, app=${appName}, device=${deviceId}`, + ); socket.on('close', () => { this._devices.delete(deviceId); @@ -206,7 +221,7 @@ class InspectorProxy { throw new Error('Incorrect URL - must provide device and page IDs'); } - const device = this._devices.get(parseInt(deviceId, 10)); + const device = this._devices.get(deviceId); if (device == null) { throw new Error('Unknown device with ID ' + deviceId); }