[Expo-go][Android] Fix Debugger #849

filip131311 · 2024-12-12T22:57:45Z

This PR solves an issue with new debugger not being attached on android in some scenarios. In #750 we introduced a way of ignoring expo go host runtime and selecting the second one when project was expo go, but for some reason the behavior of expo go runtimes seems to be inconsistent across platforms and versions and sometime the host runtime does not exist.

The following is the result of {metro}/json/list request for android expo go application in expo version: 52.0.18:

[
  {
    "id": "5ca976e985fc33959628c11becb91a7dc7842a8f-1",
    "title": "React Native Bridgeless [C++ connection]",
    "description": "host.exp.exponent",
    "type": "node",
    "devtoolsFrontendUrl": "devtools://devtools/bundled/js_app.html?experiments=true&v8only=true&ws=192.168.18.153%3A8081%2Finspector%2Fdebug%3Fdevice%3D5ca976e985fc33959628c11becb91a7dc7842a8f%26page%3D1",
    "webSocketDebuggerUrl": "ws://192.168.18.153:8081/inspector/debug?device=5ca976e985fc33959628c11becb91a7dc7842a8f&page=1",
    "deviceName": "sdk_gphone64_arm64 - 15 - API 35",
    "reactNative": {
      "logicalDeviceId": "5ca976e985fc33959628c11becb91a7dc7842a8f",
      "capabilities": {
        "prefersFuseboxFrontend": true,
        "nativeSourceCodeFetching": false,
        "nativePageReloads": true
      }
    }
  }
]

Solution

To solve the problem we introduce a new way of checking if the runtime belongs to the host application based on functionality introduced in this PR.

Additional changes

Additionally testing of expo go applications on android reveled that if application is killed, without closing the host process and then lunched again, the old runtime does not leave the {metro}/json/list endpoint. This scenario is unlikely as the user would have to do that by hand on the emulator, but to avoid problems with this bug in the future in case of expo go applications we search for the correct runtime starting form the end of the array.

How Has This Been Tested:

Run booth expo go and expo prebuilt bare application on both ios and android.

vercel · 2024-12-12T22:57:48Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
radon-ide	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 17, 2024 11:11am

packages/vscode-extension/src/project/metro.ts

maciekstosio · 2024-12-13T10:33:33Z

packages/vscode-extension/src/project/metro.ts

+      });
+
+      ws.on("error", (e) => {
+        Logger.debug(`[evaluateJsFromCdpAsync] Failed to connect ${wsUrl.toString()}`, e);


Does this happen only when the connection fails? It's a bit misleading.

we attach an error message with more details and it is true that we failed to connect

maciekstosio · 2024-12-13T10:37:34Z

packages/vscode-extension/src/project/metro.ts

@@ -292,6 +292,82 @@ export class Metro implements Disposable {
    ws.close();
  }

+  private async evaluateJsFromCdp(


I wonder if this is some generic method to call JS through CDP, shouldn't we put it in some helper instead of as part of metro?

that was my initial approach but unfortunately we need our uniq metro port that is not passed through the metro endpoints.

maciekstosio · 2024-12-13T10:38:28Z

packages/vscode-extension/src/project/metro.ts

+    source: string,
+    timeoutMs: number = 2000
+  ): Promise<string | undefined> {
+    const REQUEST_ID = 0;


I wonder if there is a reason why part of the variables is declared in the promise and rest outside, is there any reason?

if there is no reason to be defined inside the promise as for example websocket connection needs to be opened inside i lest it outside and it seems a good pattern. (I moved timeoutHandle declaration inside the promise now as it was only an empty declaration)

maciekstosio · 2024-12-13T10:42:09Z

packages/vscode-extension/src/project/metro.ts

+            }
+            settled = true;
+            clearTimeout(timeoutHandle);
+            ws.close();


settled = true; clearTimeout(timeoutHandle); ws.close();

It repeated x times, let's extract it to some method

thx nice catch

maciekstosio · 2024-12-13T10:46:53Z

packages/vscode-extension/src/project/metro.ts

+      timeoutHandle = setTimeout(() => {
+        Logger.debug(`[evaluateJsFromCdpAsync] Request timeout from ${wsUrl.toString()}`);
+        reject(new Error("Request timeout"));
+        settled = true;


That sounds redundant. Shouldn't you just close the websocket, and the on('close') with settled will decide if it's "settled" or not?

no if we did that we wouldn't know the close reason in on(close) and we want to log that

maciekstosio

One nitpick, the rest looks good, thanks for the changes!

maciekstosio · 2024-12-16T11:20:22Z

packages/vscode-extension/src/utilities/evaluateJsFromCdp.ts

+        }
+      } catch (e) {
+        reject(e);
+        settleConnection();


You always settle the connection so it can go to finally or just after try catch.

kmagiera · 2024-12-16T12:15:39Z

packages/vscode-extension/src/project/metro.ts

+        // Expo go apps using the new debugger could report more then one page,
+        // if it exist the first one being the Expo Go host runtime.
+        // more over expo go on android has a bug causing newDebuggerPages
+        // from previously run applications to leek if the host application


kmagiera · 2024-12-16T12:18:22Z

packages/vscode-extension/src/project/metro.ts

+        return true;
+      }
+    } catch (e) {
+      // we ignore errors as they wer already logged by the evaluateJsFromCdp method


kmagiera · 2024-12-16T12:18:47Z

packages/vscode-extension/src/project/metro.ts

+  private async evaluateJsFromCdp(
+    webSocketDebuggerUrl: string,
+    source: string,
+    timeoutMs: number = 2000


timeout is unused

kmagiera · 2024-12-16T12:20:16Z

packages/vscode-extension/src/project/metro.ts

+    const wsUrl = new URL(webSocketDebuggerUrl);
+    wsUrl.hostname = "localhost";
+    wsUrl.port = String(this._port);
+
+    return evaluateJsFromCdp(wsUrl.toString(), source);


this function doesn't seem particularily useful and it calls a method with exact same name which is confusing. I'd just move the websocket URL resolution to isExpoHostRuntime where it is now called. That method is short enough to accommodate additional logic

kmagiera · 2024-12-16T12:24:36Z

packages/vscode-extension/src/project/metro.ts

+        // is not. To perform this check we use expo host functionality
+        // introduced in https://github.com/expo/expo/pull/32322/files
+        for (const newDebuggerPage of newDebuggerPages.reverse()) {
+          if (!(await this.isExpoHostRuntime(newDebuggerPage.webSocketDebuggerUrl))) {


I don't think we only want to eliminate non-host runtimes here. Given we already connect to the websocket here, this method can actually verify if the runtime is active. This specifically makes sense given you write the old runtime can still be listed.

Since iseExpoHostRuntime returns false both in the case when we fail to connect with the runtime, and also in the case when we connected sucesfully to the runtime but it didn't have that flag set, I think we should actually check for the first case only. In the current form I'd assume that we'd end up selecting old runtime when the new one hasn't yet appeared on the list.

kmagiera · 2024-12-16T12:25:17Z

packages/vscode-extension/src/utilities/evaluateJsFromCdp.ts

+import WebSocket from "ws";
+import { Logger } from "../Logger";
+
+export async function evaluateJsFromCdp(


this is a weird choice for the method name. We don't evaluate "from" CDP but rather "over" CDP

…oStep1

packages/vscode-extension/src/project/metro.ts

init

270d3a8

filip131311 requested review from kmagiera and maciekstosio December 12, 2024 22:57

maciekstosio reviewed Dec 13, 2024

View reviewed changes

packages/vscode-extension/src/project/metro.ts Outdated Show resolved Hide resolved

maciekstosio reviewed Dec 13, 2024

View reviewed changes

CR

e0f13fb

vercel bot deployed to Preview December 13, 2024 10:34 View deployment

maciekstosio requested changes Dec 13, 2024

View reviewed changes

CR

9825570

filip131311 requested a review from maciekstosio December 13, 2024 12:23

vercel bot deployed to Preview December 13, 2024 12:23 View deployment

loud CR

1656f8b

vercel bot deployed to Preview December 13, 2024 13:28 View deployment

maciekstosio approved these changes Dec 16, 2024

View reviewed changes

kmagiera requested changes Dec 16, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into @Filip131311/FixExpoG…

3dac92c

…oStep1

vercel bot deployed to Preview December 17, 2024 00:28 View deployment

Small refactor to address review comments

1a58076

vercel bot deployed to Preview December 17, 2024 00:41 View deployment

maciekstosio reviewed Dec 17, 2024

View reviewed changes

packages/vscode-extension/src/project/metro.ts Outdated Show resolved Hide resolved

Update packages/vscode-extension/src/project/metro.ts

e636387

vercel bot deployed to Preview December 17, 2024 11:11 View deployment

maciekstosio approved these changes Dec 17, 2024

View reviewed changes

kmagiera approved these changes Dec 17, 2024

View reviewed changes

kmagiera merged commit d501749 into main Dec 17, 2024
4 checks passed

kmagiera deleted the @Filip131311/FixExpoGoStep1 branch December 17, 2024 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Expo-go][Android] Fix Debugger #849

[Expo-go][Android] Fix Debugger #849

filip131311 commented Dec 12, 2024 •

edited

Loading

vercel bot commented Dec 12, 2024 •

edited

Loading

maciekstosio Dec 13, 2024

filip131311 Dec 13, 2024

maciekstosio Dec 13, 2024

filip131311 Dec 13, 2024

maciekstosio Dec 13, 2024

filip131311 Dec 13, 2024

maciekstosio Dec 13, 2024

filip131311 Dec 13, 2024

maciekstosio Dec 13, 2024

filip131311 Dec 13, 2024

maciekstosio left a comment

maciekstosio Dec 16, 2024 •

edited

Loading

kmagiera Dec 16, 2024

kmagiera Dec 16, 2024

kmagiera Dec 16, 2024

kmagiera Dec 16, 2024

kmagiera Dec 16, 2024

kmagiera Dec 16, 2024

[Expo-go][Android] Fix Debugger #849

[Expo-go][Android] Fix Debugger #849

Conversation

filip131311 commented Dec 12, 2024 • edited Loading

Solution

Additional changes

How Has This Been Tested:

vercel bot commented Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maciekstosio left a comment

Choose a reason for hiding this comment

maciekstosio Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filip131311 commented Dec 12, 2024 •

edited

Loading

vercel bot commented Dec 12, 2024 •

edited

Loading

maciekstosio Dec 16, 2024 •

edited

Loading