[wrangler] Changes for the next Miniflare version #3951

mrbbot · 2023-09-14T16:54:53Z

⚠️ NOTE: this requires cloudflare/miniflare/pull/656, cloudflare/miniflare/pull/681, and cloudflare/miniflare/pull/683 to be released. I also need to verify this all works on Windows too.

What this PR solves / how to test:

Hey! 👋 This PR includes the necessary changes to Wrangler for the next Miniflare release. Best reviewed commit-by-commit, see commit's individual descriptions for more details. Notably, this PR...

Uses Miniflare's magic proxy for wrangler {kv,r2,d1} --local commands
Adds support for VSCode's breakpoint debugger to Wrangler
Adds support for breakpoint debugging in --remote and --no-bundle modes
Logs source mapped errors for uncaught exceptions in --no-bundle --remote mode
Once we've bumped Miniflare, removes the native dependency on better-sqlite3 reducing install times by ~30s on M1 if prebuilt binaries aren't found

Associated docs issue(s)/PR(s):

None yet, will want some example VSCode debugging configurations for sure.

Author has included the following, where applicable:

Tests
Changeset (Changeset guidelines)

Reviewer is to perform the following, as applicable:

Checked for inclusion of relevant tests
Checked for inclusion of a relevant changeset
Checked for creation of associated docs updates
Manually pulled down the changes and spot-tested

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

changeset-bot · 2023-09-14T16:54:56Z

🦋 Changeset detected

Latest commit: 5221a2c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
wrangler	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

mrbbot · 2023-09-14T17:06:03Z

.changeset/twelve-numbers-occur.md

+      "cwd": "/",
+      "resolveSourceMapLocations": null,
+      "attachExistingChildren": false,
+      "autoAttachChildProcesses": false,


Why do we need each of these options?

"cwd": "/"

Wrangler stores the output bundle and source maps in the OS's temporary directory. This means relative source URLs include lots of .. to break out of the temporary directory, and into the user's source folder. When receiving a relative path like this, VSCode's debugger will attempt to resolve it, but will replace / in the source map's source root with cwd: https://github.com/microsoft/vscode-js-debug/blob/bf9c6e625a799518ace4ce6adc7653174c81d3fa/src/targets/node/nodeSourcePathResolver.ts#L167

By default, cwd is the workspace root, but that leads to strange resolved source paths that include the workspace root twice. Setting / here causes /s to be replaced with /s instead, leading to correctly resolved source paths.

This is potentially something we should fix in vscode-js-debug. properResolve(fullSourceEntry) in the linked function scope above should give the correct URL.

"resolveSourceMapLocations": null

Disables source map location filtering, so maps in Wrangler's temporary directory can be found.

"attachExistingChildren": false and "autoAttachChildProcesses": false

VSCode will try to evaluate a bunch of scripts in the connected JavaScript execution context to automatically attach to spawned child processes. These scripts require Node.js APIs which workerd doesn't support. workerd also can't spawn child processes, so we don't want VSCode to try attach to them.

penalosa · 2023-09-19T10:26:13Z

packages/wrangler/src/dev/remote.tsx

+ * This URL is returned in `Debugger.scriptParsed` events, ensuring inspector
+ * clients resolve source mapping URLs correctly. It also appears in stack
+ * traces, allowing users to click through to where errors are thrown. Note,
+ * Miniflare includes similar code, so we only need to do this in remote mode.


Could miniflare rely on Wrangler doing this?

It could do, but then people using Miniflare's API directly (or Miniflare developers 😃) wouldn't be able to use VSCode/WebStorm for debugging.

penalosa · 2023-09-19T15:58:41Z

packages/wrangler/src/inspect.ts

+								sourceMaps.current.set(id, fileURLToPath(url));
+								// The hostname of this URL will show up next to the cloud icon
+								// under authored sources, so use the worker name.
+								message.params.sourceMapURL = `worker://${name}/${id}`;


Does this not show the file name as id in the deployed sources section? How does this work for scripts without a sourcemap?

We only run this code if(message.params.sourceMapURL). If not, then the //# sourceURL changes will put the full file path in the sources panel. Here's a screenshot from a --no-bundle example with a plain JavaScript file:

`miniflare` no longer provides Node APIs for accessing its persistent storage directly, as simulators are now implemented as Workers. This change updates the `wrangler kv --local` commands to use `Miniflare#getKVNamespace()` instead, proxying through the Worker simulator.

As with KV, this change updates the `wrangler kv --local` commands to use `Miniflare#getR2Bucket()` instead, proxying through the Worker simulator.

As with KV and R2, this change updates the `wrangler d1 --local` commands to use `Miniflare#getD1Database()` instead, proxying through the Worker simulator. This removes Wrangler's dependency on `better-sqlite3`, meaning we no longer have any native dependencies.

Namespace IDs are now encoded into Durable Object IDs, so we cannot check for their existence directly.

Adds `//# sourceURL` comments to source files in `--remote` dev so V8 knows where source files are on disk. This URL is returned in `Debugger.scriptParsed` events, ensuring inspector clients resolve source mapping URLs correctly. It also appears in stack traces, allowing users to click through to where errors are thrown, and making it easier for us to source map them. Note, Miniflare already includes similar code, so we only need to do this in remote mode. This is a pre-requisite to enabling breakpoint debugging in remote.

This middleware returns a machine-readable JSON response for uncaught errors. This is only intended for Miniflare's pretty error page and shouldn't be used in remote mode. We may want to add a pretty error page to remote mode eventually, but that should be a separate change.

On macOS, `os.tmpdir()` returns a symlink. This causes `esbuild` to generate invalid source maps, as the number of `../` in relative paths changes depending on whether you evaluate symlinks. We previously fixed a similar issue for the middleware loader (https://github.com/cloudflare/workers-sdk/pull/2249/files#diff-17e01c57aa611bb9e0b392a15fd63b5d18602e3a6c9a97c4a34e891bbfdcb7f3R496-R498), but the issue is more widespread than that. This change ensures all temporary directories are `realpath`ed and symlink free.

Previously, Wrangler would respond to `Network.loadNetworkResource` requests with the bundled `esbuild` source map in `--remote` mode. In local mode, Wrangler relied on Miniflare rewriting source mapping URLs to its loopback server. Unfortunately, this breaks breakpoint debugging in VSCode, so was removed in cloudflare/miniflare#681 in favour of a `//# sourceURL`-based approach. With the previous changes to include `//# sourceURL` comments in remote mode too, this means all source mapping URLs from Miniflare and remote dev will now have `file:` protocols. These cannot be fetched from our DevTools hosted on a `https:` origin. IDEs like VSCode and WebStorm expect these though. To fix hosted DevTools, this PR rewrites `Debugger.scriptParsed` events to include `worker:`-scheme URLs, only if the connected inspector client is DevTools. Wrangler will then respond to `Network.loadNetworkResource` with the source map. As noted above, Wrangler used to only respond with the source map from the internal `esbuild` bundle step. When using `--no-bundle`, users may bring their own source map**s**. Previously, these weren't served, preventing these users from using DevTools. With the new `//# sourceURL` comments, we're able to work out which source map corresponds to which source file, meaning Wrangler can now serve multiple source maps. These source maps may not include `sourcesContent` fields. In this case, DevTools sends additional `Network.loadNetworkResource` requests for sources, which Wrangler now responds to too.

When `Runtime.exceptionThrown` events are dispatched by V8, Wrangler previously only used the source map produced by `esbuild` to source map the stack trace. In `--no-bundle` mode, it's possible there might be multiple user provided source maps needed to source map the stack trace. This change switches to using the `source-map-support` package to source map stack traces. Miniflare uses this already, so we're not adding a new dependency. This relies on file URLs in stack traces, something we now have from adding `//# sourceURL` comments.

Now that we're adding `//# sourceURL` comments in remote mode, breakpoint debugging just works in remote mode, including in IDEs. This change sets the `?debugger=true` query param when opening DevTools regardless of the runtime in-use.

github-actions · 2023-09-19T19:53:02Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6240381135/npm-package-wrangler-3951

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6240381135/npm-package-wrangler-3951

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6240381135/npm-package-wrangler-3951 dev path/to/script.js

Additional artifacts:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6240381135/npm-package-cloudflare-pages-shared-3951

Note that these links will no longer work once the GitHub Actions artifact expires.

wrangler@3.8.0 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	3.20230918.0	3.20230918.0
`workerd`	1.20230904.0	1.20230904.0
`workerd --version`	1.20230904.0	2023-09-04

|

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

codecov · 2023-09-19T19:57:41Z

Codecov Report

Merging #3951 (f32d89a) into main (291c78b) will decrease coverage by 0.20%.
Report is 1 commits behind head on main.
The diff coverage is 40.92%.

❗ Current head f32d89a differs from pull request most recent head 5221a2c. Consider uploading reports for the commit 5221a2c to get more accurate results

@@            Coverage Diff             @@
##             main    #3951      +/-   ##
==========================================
- Coverage   75.08%   74.89%   -0.20%     
==========================================
  Files         195      196       +1     
  Lines       11533    11604      +71     
  Branches     3054     3044      -10     
==========================================
+ Hits         8660     8691      +31     
- Misses       2873     2913      +40

Files Changed	Coverage Δ
...ler/src/api/pages/create-worker-bundle-contents.ts	`100.00% <ø> (ø)`
packages/wrangler/src/deploy/deploy.ts	`87.33% <ø> (ø)`
...ckages/wrangler/src/pages/functions/buildPlugin.ts	`17.85% <ø> (ø)`
...ckages/wrangler/src/pages/functions/buildWorker.ts	`70.65% <ø> (ø)`
packages/wrangler/src/secret/index.ts	`87.32% <ø> (ø)`
packages/wrangler/src/sourcemap.ts	`1.69% <1.69%> (ø)`
packages/wrangler/src/dev/remote.tsx	`7.73% <6.66%> (+0.03%)`	⬆️
packages/wrangler/src/inspect.ts	`5.34% <6.66%> (+0.17%)`	⬆️
packages/wrangler/src/d1/execute.tsx	`57.55% <14.28%> (+2.45%)`	⬆️
packages/wrangler/src/dev/miniflare.ts	`65.00% <62.50%> (+2.34%)`	⬆️
... and 9 more

... and 3 files with indirect coverage changes

jspspike

The new --local changes look good to me, I can't speak as much to the source map changes but I tested them locally and they seem to be working as intended

ad6025b · 2023-09-22T17:55:21Z

@jspspike @mrbbot so with the latest miniflare, https://github.com/cloudflare/miniflare/commits/v3.20230918.0, is the step debugger in vscode fixed/available??

i notices in the /v3.20230918.0, the 2 critical PR's noted above, is not in it yet.

BTW, I test with minflare /v3.20230918.0, and the step debugger still does not work.

bompus · 2023-09-24T17:14:29Z

FYI - For me, to get breakpoints to work in VS Code, I had to add the following to the launch configuration:
"sourceMaps": true,

ad6025b · 2023-09-26T18:04:08Z

i tried with latest "miniflare": "^3.20230922.0",

and added launch configuration:
"sourceMaps": true,

it still does not work. :(

admah · 2023-09-26T18:34:56Z

@alexdoan102 this should be working as of wrangler 3.9.0. Can you go through these steps in VSCode and confirm?

add .vscode/launch.json file with contents:

{
    "configurations": [
        {
            "name": "Wrangler",
            "type": "node",
            "request": "attach",
            "port": 9229,
            "cwd": "/",
            "resolveSourceMapLocations": null,
            "attachExistingChildren": false,
            "autoAttachChildProcesses": false,
            "sourceMaps": true // works with or without this line
        }
    ]
}

run npx wrangler dev --inspectorPort 9229 - inspectorPort is optional. This just ensures the correct port is used.
set a breakpoint in your .{ts/js} file
launch 'run and debug' command "Wrangler" (the one configured in your launch.json)
make a request to the worker url (default: http://localhost:8787/) via the browser or curl

** Note: ** make sure there are no other Wrangler processes running when you do this.

ad6025b · 2023-09-28T10:32:04Z

@admah it does work, but there is a BUG ==> i reported the BUG: https://github.com/cloudflare/workers-sdk/issues/4052]

in Windows, if your source code is a different hard drive (d:), than your Windows drive (c:), the breakpoints are NEVER hit, and it does not break.

root cause ==> wrangler cannot find the sourcemaps at %tmp% ==> it assumes the path to the sourcemaps is on the same hard drive. (you must include the hard drive paths when searching for the sourcemaps)

#3951 introduced a change that closed the bootstrapper's IPC channel when Wrangler exited. Whenever `wrangler (pages) dev` reloads, an event is sent on this channel to indicate the dev server is ready. Unfortunately, `wrangler pages dev`'s handler doesn't wait for the dev session to exit before resolving its returned promise, meaning the channel is closed as soon as the dev server is ready, not when Wrangler exits. This means that ready events sent from reloads result in an `ERR_IPC_CHANNEL_CLOSED` that crashes Wrangler. :(

In #3951, we added a [`finally` block](https://github.com/cloudflare/workers-sdk/blob/b5e62b931ad6671e4dce9444a279bb3ec8f63991/packages/wrangler/src/index.ts#L796-L802) to ensure that the wrangler CLI child process disconnects from the IPC channel connecting it to the parent process. Since the parent process forwards any messages it receives on the child IPC channel up to any higher-level IPC channel, we should follow suit and also disconnect any higher-level IPC channel as well.

…4267) In #3951, we added a [`finally` block](https://github.com/cloudflare/workers-sdk/blob/b5e62b931ad6671e4dce9444a279bb3ec8f63991/packages/wrangler/src/index.ts#L796-L802) to ensure that the wrangler CLI child process disconnects from the IPC channel connecting it to the parent process. Since the parent process forwards any messages it receives on the child IPC channel up to any higher-level IPC channel, we should follow suit and also disconnect any higher-level IPC channel as well.

mrbbot commented Sep 14, 2023

View reviewed changes

penalosa reviewed Sep 19, 2023

View reviewed changes

mrbbot requested a review from penalosa September 19, 2023 17:40

mrbbot marked this pull request as ready for review September 19, 2023 18:35

mrbbot requested review from a team as code owners September 19, 2023 18:35

mrbbot added 10 commits September 19, 2023 14:45

Use Miniflare's magic proxy for wrangler r2 --local commands

ed42fba

As with KV, this change updates the `wrangler kv --local` commands to use `Miniflare#getR2Bucket()` instead, proxying through the Worker simulator.

Update pages-workerjs-directory tests for new persistence layout

7951fdf

Namespace IDs are now encoded into Durable Object IDs, so we cannot check for their existence directly.

jspspike force-pushed the bcoll/tre-next-miniflare-fixups branch from 1c0d541 to f32d89a Compare September 19, 2023 19:50

Bump miniflare version to 3.20230918

5221a2c

jspspike force-pushed the bcoll/tre-next-miniflare-fixups branch from f32d89a to 5221a2c Compare September 19, 2023 20:04

jspspike approved these changes Sep 19, 2023

View reviewed changes

jspspike merged commit e0850ad into main Sep 19, 2023
14 checks passed

jspspike deleted the bcoll/tre-next-miniflare-fixups branch September 19, 2023 20:32

github-actions bot mentioned this pull request Sep 19, 2023

Version Packages #3949

Merged

ad6025b mentioned this pull request Sep 28, 2023

🐛 BUG: VSCode breakpoint debugging fails to load sources on non C:/ drives #4052

Closed

mrbbot mentioned this pull request Sep 28, 2023

[wrangler] fix: allow wrangler pages dev sessions to be reloaded #4054

Merged

2 tasks

1000hz mentioned this pull request Oct 23, 2023

Disconnect parent process IPC channel when child process disconnects #4267

Merged

mrbbot mentioned this pull request Nov 7, 2023

🐛 BUG: cache.put causes errors locally on wrangler 3.5.0 #3725

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wrangler] Changes for the next Miniflare version #3951

[wrangler] Changes for the next Miniflare version #3951

mrbbot commented Sep 14, 2023 •

edited

Loading

changeset-bot bot commented Sep 14, 2023 •

edited

Loading

mrbbot Sep 14, 2023 •

edited

Loading

penalosa Sep 19, 2023

mrbbot Sep 19, 2023 •

edited

Loading

penalosa Sep 19, 2023

mrbbot Sep 19, 2023 •

edited

Loading

github-actions bot commented Sep 19, 2023 •

edited

Loading

codecov bot commented Sep 19, 2023 •

edited

Loading

jspspike left a comment

ad6025b commented Sep 22, 2023

bompus commented Sep 24, 2023

ad6025b commented Sep 26, 2023 •

edited

Loading

admah commented Sep 26, 2023 •

edited

Loading

ad6025b commented Sep 28, 2023

[wrangler] Changes for the next Miniflare version #3951

[wrangler] Changes for the next Miniflare version #3951

Conversation

mrbbot commented Sep 14, 2023 • edited Loading

changeset-bot bot commented Sep 14, 2023 • edited Loading

🦋 Changeset detected

mrbbot Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

"cwd": "/"

"resolveSourceMapLocations": null

"attachExistingChildren": false and "autoAttachChildProcesses": false

penalosa Sep 19, 2023

Choose a reason for hiding this comment

mrbbot Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

penalosa Sep 19, 2023

Choose a reason for hiding this comment

mrbbot Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 19, 2023 • edited Loading

codecov bot commented Sep 19, 2023 • edited Loading

Codecov Report

jspspike left a comment

Choose a reason for hiding this comment

ad6025b commented Sep 22, 2023

bompus commented Sep 24, 2023

ad6025b commented Sep 26, 2023 • edited Loading

admah commented Sep 26, 2023 • edited Loading

ad6025b commented Sep 28, 2023

mrbbot commented Sep 14, 2023 •

edited

Loading

changeset-bot bot commented Sep 14, 2023 •

edited

Loading

mrbbot Sep 14, 2023 •

edited

Loading

`"cwd": "/"`

`"resolveSourceMapLocations": null`

`"attachExistingChildren": false` and `"autoAttachChildProcesses": false`

mrbbot Sep 19, 2023 •

edited

Loading

mrbbot Sep 19, 2023 •

edited

Loading

github-actions bot commented Sep 19, 2023 •

edited

Loading

codecov bot commented Sep 19, 2023 •

edited

Loading

ad6025b commented Sep 26, 2023 •

edited

Loading

admah commented Sep 26, 2023 •

edited

Loading