Expo go detaching debugger alternative fix #994

maciekstosio · 2025-02-28T10:42:34Z

This PR fixes issue where debugger is detached after restart, when runing Expo Go app. This is due to the fact that when reloading metro, the new instance of debugger with new websocket adress is created. This is alternate solution to: #985

This PR solves it as follows:

after running metro.reload() we wait for app to be ready
then we get list of available runtimes and theire WS addresses
we check if current vscSession is still on list from above
if not we close the connection, and start again

How Has This Been Tested:

For debugger that detaches during restart i.e. expo-go test app:

Lunch expo-go app
Test if debugger works (for example log or break point)
Restart with Reload JS
Wait until it restarts, and make sure the debugger is still attached (there should be also nice ui with "launching", "attaching to debugger")

For apps that debugger do not detach i.e. react-native-76

Lunch expo-go app
Test if debugger works (for example log or break point)
Add breakpoint in

radon-ide/packages/vscode-extension/src/debugging/DebugSession.ts

Line 107 in 59372cc

return this.connectJSDebugger(metro);
Restart with Reload JS
Wait until it restarts (it should not stop at the breakpoint)
Make sure the debugger is still attached (the UI should be similar as above, but the "attaching debugger" will pass in the blink of an eye)

vercel · 2025-02-28T10:42:37Z

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	Mar 4, 2025 0:52am

…lternative-fix

filip131311

I left an inline comment, more over would you mind adding an information to the PR description that the changes relate to the old debugger, seems like something that might be important in the future

filip131311 · 2025-03-03T12:27:36Z

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

+        return listJson;
+      }
+
+      await sleep(progressiveRetryTimeout(retryCount));


I don't understand this code, it seems to only be reachable when the fetch above fails, but that would throw uncaught rejection anyway preventing any future retries, wouldn't it? does this condition: listJson.length happen often? If so could you add a comment explaining it?

That being said it seems the fetch above should be wrapped in try catch block

Not only, basically we're pooling the variables. The list can be empty at the beginning.

Previously we did it in a way that we we're retraing entrie fetchDebuggerURL, but my understanding is that it could fail only when fetch was empty. I needed to have method that fetches the list separatly, so we don't open the websocket to runtime, as it closes previous connection, thus I moved the retry logic to other place, but I belive the entrie process will work as before.

Nevertheless, I agree we should wrap it in try catch block, good catch!

Changed in: 9aaf04d

filip131311

Looks good now

…lternative-fix

kmagiera · 2025-03-03T20:13:15Z

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

-    // query list from http://localhost:${metroPort}/json/list
-    const list = await fetch(`http://localhost:${this._port}/json/list`);
-    const listJson = await list.json();
+  public async fetchRuntimeList(): Promise<CDPTargetDescription[] | undefined> {


It doesn't list runtimes here but websocket targets. This is a more abstract concept as a single target actually may cover many runtimes. Specifically this happens when you do "reload JS" in react native – new runtime is created but it is listed under the same WS target.

I'd rename to fetchCDPWebsocketTargets or something simpler by removing "CDP" or "Websocket" fragments

kmagiera · 2025-03-03T20:17:36Z

packages/vscode-extension/src/debugging/DebugSession.ts

+      return this.start();
+    }
+
+    return false;


I commented about this in the other PR, but I don't like that this method returns false in two following scenarios:

when there's no need to reconnect

when there's need to reconnect and we fail to reconnect properly

Then, this method returns true only when we need to reconnect and that was successful.

I'd feel more natural to return true always when there's a proper connection established regardless of whether we needed to reconnect or not, and then return false in case we couldn't establish the connection

Yeah, I changed it there, but checkout this branch earlier.

kmagiera · 2025-03-03T20:21:29Z

packages/vscode-extension/src/debugging/DebugSession.ts

+    const availableAddresses =
+      possibleRuntimes?.map((runtime) => runtime.webSocketDebuggerUrl) ?? [];
+
+    if (!availableAddresses.includes(this.vscSession?.configuration?.websocketAddress)) {


this seems overly complicated I think you just want to find runtime with the given ws URL, so this should be something like:

const currentWsAddress = this.vscSession?.configuration?.websocketAddress; const hasCurrentWsAddress = possibleRuntimes?.some(runtime => runtime.webSocketDebuggerUrl === currentWsAddress)

kmagiera

I left a couple inline comments outside of this review. Please address them prior to merging.

I also think it'd be appropriate for the test plan to cover scenarios w/o expo-go. I understand this PR changes behavior in those cases too. Specifically we should verify that we don't reconnect in those setups. We will also be showing different states in the UI when refresh happens from what I understand?

maciekstosio · 2025-03-04T08:03:31Z

I left a couple inline comments outside of this review. Please address them prior to merging.

I also think it'd be appropriate for the test plan to cover scenarios w/o expo-go. I understand this PR changes behavior in those cases too. Specifically we should verify that we don't reconnect in those setups. We will also be showing different states in the UI when refresh happens from what I understand?

Yes, I'll add information about checking for regression, but the flow will be more or less the same. By more or less I mean that for other setups overlay with "attaching debugger" will pass in the blink of an eye, but will be checked anyway.

filip131311 · 2025-03-05T11:23:12Z

The other solution in #985 won

This PR fixes issue where debugger is detached after restart, when runing Expo Go app. This is due to the fact that when reloading metro, the new instance of debugger with new websocket adress is created. This PR solves it as follows: - after running `metro.reload()` we wait for app to be ready - once it's ready, we're checking if old debug session is available - when old runtime is not available we close old debug session and start the now one - if old runtime is available, then we do nothing How to determine the old debug session is available: So there are to approches: - trying to execture something through the debugger - check if old debugger is on the list (#994) The problem with the first one is that when debuger is stale, it might just not answer, in that case we timeout after 1 sec. The problem with the second one is that sometimes there is ws target available that can't be used (Expo Go case), this the solution is not working reliably. To overcome those two problems we combined both solutions. We run both checks in `promise.any`, so: - If ping returns true, we're good (connection alive) - if checking list of ws targets returns false, we're good (connection closed) - if current ws target is on the list, we reject so `promise.any` waits for ping and ping will check that reliably after at most 1 sec ### How Has This Been Tested: **For debugger that detaches during restart i.e. expo-go test app:** - Lunch expo-go app - Test if debugger works (for example log or break point) - Restart with `Reload JS` - Wait until it restarts, and make sure the debugger is still attached (there should be also nice ui with "launching", "attaching to debugger") **For apps that debugger do not detach i.e. react-native-76** - Lunch expo-go app - Test if debugger works (for example log or break point) - Add breakpoint in https://github.com/software-mansion/radon-ide/blob/59372cc4e83d28edb2ae6f183b2f0d0a2e48eeb9/packages/vscode-extension/src/debugging/DebugSession.ts#L107 - Restart with `Reload JS` - Wait until it restarts (**it should not stop at the breakpoint**) - Make sure the debugger is still attached (the UI should be similar as above, but the "attaching debugger" will pass in the blink of an eye) |Expo Go Before|Expo Go After| |-|-| |<video src="https://github.com/user-attachments/assets/5ae32a1c-4c33-4a55-98a1-c645f4e92e59" />|<video src="https://github.com/user-attachments/assets/eca7c8c2-8806-45c7-a386-10f79dd706d7" />| |RN76 Before|RN 76 After| |-|-| |<video src="https://github.com/user-attachments/assets/e0b81615-e2f7-412a-b3e5-6f83e141b9a7" />|<video src="https://github.com/user-attachments/assets/70b6fae8-8ebe-4ba3-bc5d-141c9dc545ef" />|

maciekstosio added 8 commits February 21, 2025 17:32

try to reconnect after metro reload

c979021

improve messaging, progressing interval,

cb05694

update prop name

11643eb

fix lint

7e9b48c

chore rename method

4cd1cf0

add comment about unusual use

fafbe07

Fix lint

b0dc68c

alternative approche with list the available runtimes list

f068503

maciekstosio changed the title ~~@maciekstosio/expo go detaching debugger alternative fix~~ Expo go detaching debugger alternative fix Feb 28, 2025

maciekstosio marked this pull request as draft February 28, 2025 11:03

fix getting the list of runtimes

c03eadd

maciekstosio marked this pull request as ready for review February 28, 2025 16:34

vercel bot deployed to Preview February 28, 2025 16:54 View deployment

maciekstosio mentioned this pull request Mar 3, 2025

Fix debugger detaching when restarting on Expo Go #985

Merged

Merge branch 'main' into @maciekstosio/Expo-Go-detaching-debugger---a…

0208195

…lternative-fix

vercel bot deployed to Preview March 3, 2025 12:20 View deployment

filip131311 requested changes Mar 3, 2025

View reviewed changes

Update metro.ts

9aaf04d

vercel bot deployed to Preview March 3, 2025 14:41 View deployment

Update metro.ts

98aa5b6

vercel bot deployed to Preview March 3, 2025 14:48 View deployment

filip131311 approved these changes Mar 3, 2025

View reviewed changes

Merge branch 'main' into @maciekstosio/Expo-Go-detaching-debugger---a…

eb206ca

…lternative-fix

vercel bot deployed to Preview March 3, 2025 17:30 View deployment

Merge branch 'main' into @maciekstosio/Expo-Go-detaching-debugger---a…

4d1ba44

…lternative-fix

kmagiera reviewed Mar 3, 2025

View reviewed changes

kmagiera approved these changes Mar 3, 2025

View reviewed changes

adjust to recent changes

a924946

vercel bot deployed to Preview March 4, 2025 08:04 View deployment

fix prettier

59372cc

vercel bot deployed to Preview March 4, 2025 08:16 View deployment

reuse websocket addres fixup

7fbc1da

vercel bot deployed to Preview March 4, 2025 12:52 View deployment

filip131311 closed this Mar 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expo go detaching debugger alternative fix #994

Expo go detaching debugger alternative fix #994

maciekstosio commented Feb 28, 2025 •

edited

Loading

vercel bot commented Feb 28, 2025 •

edited

Loading

filip131311 left a comment

filip131311 Mar 3, 2025

maciekstosio Mar 3, 2025

maciekstosio Mar 3, 2025

filip131311 left a comment

kmagiera Mar 3, 2025

kmagiera Mar 3, 2025

maciekstosio Mar 4, 2025

kmagiera Mar 3, 2025

kmagiera left a comment

maciekstosio commented Mar 4, 2025

filip131311 commented Mar 5, 2025

Expo go detaching debugger alternative fix #994

Expo go detaching debugger alternative fix #994

Conversation

maciekstosio commented Feb 28, 2025 • edited Loading

How Has This Been Tested:

vercel bot commented Feb 28, 2025 • edited Loading

filip131311 left a comment

Choose a reason for hiding this comment

filip131311 Mar 3, 2025

Choose a reason for hiding this comment

maciekstosio Mar 3, 2025

Choose a reason for hiding this comment

maciekstosio Mar 3, 2025

Choose a reason for hiding this comment

filip131311 left a comment

Choose a reason for hiding this comment

kmagiera Mar 3, 2025

Choose a reason for hiding this comment

kmagiera Mar 3, 2025

Choose a reason for hiding this comment

maciekstosio Mar 4, 2025

Choose a reason for hiding this comment

kmagiera Mar 3, 2025

Choose a reason for hiding this comment

kmagiera left a comment

Choose a reason for hiding this comment

maciekstosio commented Mar 4, 2025

filip131311 commented Mar 5, 2025

maciekstosio commented Feb 28, 2025 •

edited

Loading

vercel bot commented Feb 28, 2025 •

edited

Loading