feature(cli): allow client-side device ids to reuse debugger sessions…

… when restarting app (#22742)

# Why

After using the debugger for some time, I noticed a really annoying
behavior when the app crashes or is restarted manually. See
facebook/metro#985 for more info.

# How

This adds support for client-side unique device identifiers, such as
"installation ids" or "device ids". Similarly to future support within
Metro: facebook/metro#991

# Test Plan

See updated tests, and the test plan at
facebook/metro#991

> - 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_

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [ ] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [ ] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).

packages/@expo/cli/CHANGELOG.md

@@ -10,6 +10,7 @@
 
 - Add `-g, --go` option to `expo start` to force using Expo Go by default. ([#22925](https://github.com/expo/expo/pull/22925) by [@EvanBacon](https://github.com/EvanBacon))
 - Add `-d` as an alias to `--dev-client`. ([#22925](https://github.com/expo/expo/pull/22925) by [@EvanBacon](https://github.com/EvanBacon))
+- Allow client-side device ids to reuse debugger sessions when restarting app. ([#22742](https://github.com/expo/expo/pull/22742) by [@byCedric](https://github.com/byCedric))
 
 ### 🐛 Bug fixes
 

packages/@expo/cli/src/start/server/metro/inspector-proxy/__tests__/proxy.test.ts

@@ -78,6 +78,25 @@ it('creates a new device when a client connects', async () => {
   }
 });
 
+it('creates a new device with client-side device id when a client connects', async () => {
+  const { expoProxy } = createTestProxy();
+  const { server, deviceWebSocketUrl } = await createTestServer();
+  useWebsockets(server, expoProxy.createWebSocketListeners(server));
+
+  const device = new WS(`${deviceWebSocketUrl}?device=someuniqueid`);
+
+  try {
+    await new Promise((resolve) => device.on('open', resolve));
+
+    expect(device.readyState).toBe(device.OPEN);
+    expect(expoProxy.devices.size).toBe(1);
+    expect(expoProxy.devices.get('someuniqueid')).toBeDefined();
+  } finally {
+    server.close();
+    device.close();
+  }
+});
+
 it('removes device when client disconnects', async () => {
   const { expoProxy } = createTestProxy();
   const { server, deviceWebSocketUrl } = await createTestServer();
@@ -119,7 +138,7 @@ it('accepts debugger connections when device is connected', async () => {
 
     const deviceDebugHandler = jest.spyOn(device, 'handleDebuggerConnection');
 
-    debuggerWs = new WS(`${debuggerWebSocketUrl}?device=${device.id}&page=1`);
+    debuggerWs = new WS(`${debuggerWebSocketUrl}?device=${device._id}&page=1`);
     await new Promise((resolve) => debuggerWs?.on('open', resolve));
 
     expect(debuggerWs.readyState).toBe(debuggerWs.OPEN);
@@ -131,10 +150,71 @@ it('accepts debugger connections when device is connected', async () => {
   }
 });
 
+it('keeps debugger connection alive when device reconnects', async () => {
+  const { expoProxy } = createTestProxy();
+  const { server, deviceWebSocketUrl, debuggerWebSocketUrl } = await createTestServer();
+  useWebsockets(server, expoProxy.createWebSocketListeners(server));
+
+  let oldDeviceWs: WS | null = null;
+  let newDeviceWs: WS | null = null;
+  let debuggerWs: WS | null = null;
+
+  try {
+    // Connect the "old" device first
+    oldDeviceWs = new WS(`${deviceWebSocketUrl}?device=samedevice`);
+    await new Promise((resolve) => oldDeviceWs?.on('open', resolve));
+
+    const oldDevice = expoProxy.devices.get('samedevice');
+    expect(oldDevice).toBeDefined();
+
+    // Connect the debugger
+    const deviceDebugHandler = jest.spyOn(oldDevice!, 'handleDebuggerConnection');
+
+    debuggerWs = new WS(`${debuggerWebSocketUrl}?device=samedevice&page=1`);
+    await new Promise((resolve) => debuggerWs?.on('open', resolve));
+
+    expect(debuggerWs.readyState).toBe(debuggerWs.OPEN);
+    expect(deviceDebugHandler).toBeCalled();
+
+    // Reconnect the device using the "new" device connection
+    newDeviceWs = new WS(`${deviceWebSocketUrl}?device=samedevice`);
+
+    // Wait until both sockets have updated
+    await Promise.all([
+      new Promise((resolve) => oldDeviceWs?.on('close', resolve)),
+      new Promise((resolve) => newDeviceWs?.on('open', resolve)),
+    ]);
+
+    const newDevice = expoProxy.devices.get('samedevice');
+    expect(newDevice).not.toBe(oldDevice);
+    expect(expoProxy.devices.size).toBe(1);
+
+    // Check if the debugger and new device connections are still open
+    expect(debuggerWs.readyState).toBe(debuggerWs.OPEN);
+    expect(newDeviceWs.readyState).toBe(newDeviceWs.OPEN);
+    expect(oldDeviceWs.readyState).toBe(oldDeviceWs.CLOSED);
+  } finally {
+    server.close();
+    oldDeviceWs?.close();
+    newDeviceWs?.close();
+    debuggerWs?.close();
+  }
+});
+
 function createTestProxy() {
   class ExpoDevice {
-    constructor(public readonly id: number) {}
+    constructor(
+      public readonly _id: string,
+      public readonly _name: string,
+      public readonly _app: string,
+      public readonly _deviceSocket: WS
+    ) {}
+
     handleDebuggerConnection() {}
+
+    handleDuplicateDeviceConnection() {
+      this._deviceSocket.close();
+    }
   }
 
   const metroProxy = new MetroProxy();

packages/@expo/cli/src/start/server/metro/inspector-proxy/device.ts

@@ -30,6 +30,28 @@ export function createInspectorDeviceClass(
       return this.handlers.some((handler) => handler.onDebuggerMessage?.(message, info) ?? false);
     }
 
+    /**
+     * Handle a new device connection with the same device identifier.
+     * When the app and device name matches, we can reuse the debugger connection.
+     * Else, we have to shut the debugger connection down.
+     */
+    handleDuplicateDeviceConnection(newDevice: InstanceType<typeof MetroDeviceClass>) {
+      if (this._app !== newDevice._app || this._name !== newDevice._name) {
+        this._deviceSocket.close();
+        this._debuggerConnection?.socket.close();
+        return;
+      }
+
+      const oldDebugger = this._debuggerConnection;
+      this._debuggerConnection = null;
+
+      if (oldDebugger) {
+        oldDebugger.socket.removeAllListeners();
+        this._deviceSocket.close();
+        newDevice.handleDebuggerConnection(oldDebugger.socket, oldDebugger.pageId);
+      }
+    }
+
     /** Hook into the message life cycle to answer more complex CDP messages */
     async _processMessageFromDevice(message: DeviceRequest<any>, info: DebuggerInfo) {
       if (!this.onDeviceMessage(message, info)) {

packages/@expo/cli/src/start/server/metro/inspector-proxy/proxy.ts

@@ -19,9 +19,11 @@ export class ExpoInspectorProxy<D extends MetroDevice = MetroDevice> {
   constructor(
     public readonly metroProxy: MetroProxy,
     private DeviceClass: Instantiatable<D>,
-    public readonly devices: Map<number, D> = new Map()
+    public readonly devices: Map<string, D> = new Map()
   ) {
     // monkey-patch the device list to expose it within the metro inspector
+    // See https://github.com/facebook/metro/pull/991
+    // @ts-expect-error - Device ID is changing from `number` to `string`
     this.metroProxy._devices = this.devices;
 
     // force httpEndpointMiddleware to be bound to this proxy instance
@@ -73,19 +75,36 @@ export class ExpoInspectorProxy<D extends MetroDevice = MetroDevice> {
     // See: https://github.com/facebook/metro/blob/eeb211fdcfdcb9e7f8a51721bd0f48bc7d0d211f/packages/metro-inspector-proxy/src/InspectorProxy.js#L157
     wss.on('connection', (socket, request) => {
       try {
-        const deviceId = this.metroProxy._deviceCounter++;
-        const { deviceName, appName } = getNewDeviceInfo(request.url);
+        const fallbackDeviceId = String(this.metroProxy._deviceCounter++);
+        const { deviceId: newDeviceId, deviceName, appName } = getNewDeviceInfo(request.url);
 
-        this.devices.set(
+        const deviceId = newDeviceId ?? fallbackDeviceId;
+
+        const oldDevice = this.devices.get(deviceId);
+        const newDevice = new this.DeviceClass(
           deviceId,
-          new this.DeviceClass(deviceId, deviceName, appName, socket, this.metroProxy._projectRoot)
+          deviceName,
+          appName,
+          socket,
+          this.metroProxy._projectRoot
         );
 
-        debug('New device connected: device=%s, app=%s', deviceName, appName);
+        if (oldDevice) {
+          debug('Device reconnected: device=%s, app=%s, id=%s', deviceName, appName, deviceId);
+          // See: https://github.com/facebook/metro/pull/991
+          // @ts-expect-error - Newly introduced method coming to metro-inspector-proxy soon
+          oldDevice.handleDuplicateDeviceConnection(newDevice);
+        } else {
+          debug('New device connected: device=%s, app=%s, id=%s', deviceName, appName, deviceId);
+        }
+
+        this.devices.set(deviceId, newDevice);
 
         socket.on('close', () => {
-          this.devices.delete(deviceId);
-          debug('Device disconnected: device=%s, app=%s', deviceName, appName);
+          if (this.devices.get(deviceId) === newDevice) {
+            this.devices.delete(deviceId);
+            debug('Device disconnected: device=%s, app=%s, id=%s', deviceName, appName, deviceId);
+          }
         });
       } catch (error: unknown) {
         let message = '';
@@ -124,7 +143,7 @@ export class ExpoInspectorProxy<D extends MetroDevice = MetroDevice> {
           throw new Error(`Missing "device" and/or "page" IDs in query parameters`);
         }
 
-        const device = this.devices.get(parseInt(deviceId, 10));
+        const device = this.devices.get(deviceId);
         if (!device) {
           // TODO(cedric): change these errors to proper error types
           throw new Error(`Device with ID "${deviceId}" not found.`);
@@ -165,6 +184,7 @@ function asString(value: string | string[] = ''): string {
 function getNewDeviceInfo(url: IncomingMessage['url']) {
   const { query } = parse(url ?? '', true);
   return {
+    deviceId: asString(query.device) || undefined,
     deviceName: asString(query.name) || 'Unknown device name',
     appName: asString(query.app) || 'Unknown app name',
   };