Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix step-debugging in Workerd #1480

Merged
merged 16 commits into from
Nov 15, 2023
Merged

Fix step-debugging in Workerd #1480

merged 16 commits into from
Nov 15, 2023

Conversation

frandiox
Copy link
Contributor

@frandiox frandiox commented Nov 2, 2023

This PR enables step-debugging when using --worker-unstable in combination with --debug, as well as allowing external DevTools to connect to the workerd inspector.

image

The approach is a simplified version of what Wrangler does: it adds a server proxy between the workerd and the local debugger which:

To test the debugger in VSCode, add this to .vscode/launch.json and start the debugger after the app is running with h2 dev --worker-unstable --debug.

{
  "version": "0.2.0",
  "configurations": [
    {
      "name": "Hydrogen",
      "type": "node",
      "request": "attach",
      "port": 9229,
      "cwd": "/",
      "resolveSourceMapLocations": null,
      "attachExistingChildren": false,
      "autoAttachChildProcesses": false,
      "restart": true,
    }
  ]
}

To test Chrome DevTools, open chrome://inspect and make sure the port 9222 is added to the network targets.

image

@frandiox frandiox changed the title Fix step-debugging Fix step-debugging in Workerd Nov 9, 2023
@frandiox frandiox requested a review from a team November 9, 2023 02:57
@frandiox frandiox marked this pull request as ready for review November 9, 2023 02:57

This comment has been minimized.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code in this file was extracted from mini-oxygen/workerd-inspector.ts and hasn't changed at all. Feel free to skip it in the review.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀👀👀👀 😜

Copy link
Contributor

@blittle blittle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified it works in both VSCode and Chrome. A few thoughts:

  1. It does not work with a "JavaScript Debug Terminal" presumably because VSCode isn't connecting to the right port. That's unfortunate, because the JS debug terminal is convenient without needing to setup a vscode launch config. I often prefer this because the port itself is variable (if something's already on port 3000, it goes to 3001). But probably it's not that likely to boot up multiple hydrogen apps at the same time? If so, perhaps down the road we could add a CLI init question around VSCode (do you use VSCode?). And if so, then automatically generate the launch config?
    image

  2. Not in scope for this work, but it would be great if we could also support something like --debug-brk to automatically pause at startup.

@@ -8,6 +8,7 @@
"ci:checks": "turbo run lint test format:check typecheck",
"dev": "npm run dev:pkg",
"dev:pkg": "cross-env LOCAL_DEV=true turbo dev --parallel --filter=./packages/*",
"dev:app": "cd templates/skeleton && cross-env LOCAL_DEV=true npm run dev",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼

...(debug
? [
{
warn: `\n\nDebugger listening on ws://localhost:${inspectorPort}`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is important, because the default port that chrome looks for is different than what we default to. I had to manually add 9222 for chrome to see it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my browser, both 9222 and 9229 are by default added. However, VSCode only uses 9229 by default so I'm going to swap these 2 ports to make 9229 the public one and 9222 the private one for workerd 👍

image

const url = req.url?.split('?')[0] ?? '';

switch (url) {
// We implement a couple of well known end points
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious how you knew how to do this?

Copy link
Contributor Author

@frandiox frandiox Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of this comes from Wrangler itself! But also it's documented in https://chromedevtools.github.io/devtools-protocol/#endpoints

blittle
blittle approved these changes Nov 14, 2023
@frandiox
Copy link
Contributor Author

Thanks for the review!

It does not work with a "JavaScript Debug Terminal" presumably because VSCode isn't connecting to the right port.

Unfortunately, the JavaScript Debug Terminal can only attach to Node.js processes, it doesn't support connecting to an independent port :/

image

And if so, then automatically generate the launch config?

This is interesting, maybe we could do this automatically if we detect a .vscode folder at the root when using --debug? 🤔

Not in scope for this work, but it would be great if we could also support something like --debug-brk to automatically pause at startup.

I don't think we can make this work in workerd at the moment.
We could try sending an artificial Debugger.setBreakpoint event pointing to the first line of the worker (outside of fetch). However, we'd need to wait for workerd to send us first a Debugger.scriptParsed, and I think at that point the code in the outer-scope has already run (or will run soon enough and won't wait for our breakpoint event).

@frandiox frandiox merged commit 2bff9fc into main Nov 15, 2023
9 checks passed
@frandiox frandiox deleted the fd-step-debugging branch November 15, 2023 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants