startDevWorker Milestone 1 - Reboot #4413

RamIdeas · 2023-11-09T13:49:41Z

What this PR solves / how to test:

This PR is for all changes made since Milestone 1 was first merged in #3960

Notable commits:

bedbe4e fix: don't show logs to ProxyWorker(s) unless log level is debug
- fixes 🐛 BUG: ProxyWorker request logs #4168
76f90ed fix: show console.log's in remote mode remote inspector websocket upgrade request required auth headers so use fetch with Upgrade: websocket header instead of new WebSocket
- fixes 🐛 BUG: wrangler dev --remote no longer logs? #4165
1271a1a use miniflare verbose mode only if debug log level
- should reduce "scary logs" noise
7198a81 register ProxyWorker with DevRegistry instead of UserWorker
f09c51d Revert "register ProxyWorker with DevRegistry"
- this commit has been applied+reverted and will be reapplied in Milestone 2 due to its dependence on the new bindings shape
- it is an optimisation for the devregistry and functionally behaves the same
1a9e2f5 use single Miniflare instance for (Inspector)ProxyWorker
- fixes DEVX-929
997d77d port: clear remote runtime logs upon UserWorker restarts
- this forward-ports this change
6129d96 default unstable_dev inspectorPort to 0
- this is a more sane default for unstable_dev which is used most in test suites and therefore most likely run in parallel
7ac5efd parallelise cleanup to minimise chance of hanging
- previously, sequential cleanups fail to fully cleanup if earlier steps in the sequence fail
974216e ensure InspectorProxyWorker unsafeDirectPort is set
- undefined disables the inspector – we should implement a separate option to explicitly enable/disable the inspector in Miniflare
001c542 don't use file-system for (Inspector)ProxyWorker DOs
- the DOs don't use storage so we can tell Miniflare to not bother setting up the tmp dirs
3fea268 prevent eviction of the Durable Objects with (Inspector)ProxyWorker
- Durable Object eviction was implemented and shipped during this work which caused the in-memory state to disappear across requests (interferring with pause/play control messages) – since these DOs are for local development, we can disable eviction entirely
183db24 ready event should await proxyWorker.setOptions
- workaround for miniflare bug
7f768e6 remove miniflare workaround for parallel requests
- removes a previous workaround
f72497e considerations for race between control messages and user fetches
- this guarantees the order of requests for control messages and worker.fetch requests which are more likely to run into a race condition over the async http boundary than eye-ball requests
0845c9d use port: undefined vs 0 for UserWorker to force different port across reloads to workaround workerd bug on Windows
- this is a workaround for 🐛 Bug Report — Runtime APIs: first request to restarted workerd instance on same port from workerd fails with Internal Server Error on Windows workerd#1376
217d8ba Don't try to parse node internals
- this fixes an issue with devtools not working introduced by Implement Node-like console.log()ing workerd#1294
a49cc8a only proxy consoleAPICalled events in remote mode
a1d5afa enable consoleAPICalled events proxying if local mode AND service-worker format
- these 2 commits account for workerd now printing console.log's itself since Implement Node-like console.log()ing workerd#1294

Author has addressed the following:

Tests
- Included
- Not necessary because:
Changeset (Changeset guidelines)
- Included
- Not necessary because:
Associated docs
- Issue(s)/PR(s): linked above
- Not necessary because:

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-11-09T13:49:45Z

🦋 Changeset detected

Latest commit: 7506c8f

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

github-actions · 2023-11-09T13:52:44Z

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/6962539379/npm-package-wrangler-4413

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

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

Or you can use npx with this latest build directly:

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

Additional artifacts:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6962539379/npm-package-miniflare-4413

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

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

[email protected] includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20231030.1
`workerd`	1.20231030.0	1.20231030.0
`workerd --version`	1.20231030.0	2023-10-30

|

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

codecov · 2023-11-16T15:28:47Z

Codecov Report

❗ No coverage uploaded for pull request base (startDevWorker-release@3dd2a65). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head e82dbcd differs from pull request most recent head 7506c8f. Consider uploading reports for the commit 7506c8f to get more accurate results

Additional details and impacted files

@@                    Coverage Diff                    @@
##             startDevWorker-release    #4413   +/-   ##
=========================================================
  Coverage                          ?   75.48%           
=========================================================
  Files                             ?      240           
  Lines                             ?    12851           
  Branches                          ?     3309           
=========================================================
  Hits                              ?     9701           
  Misses                            ?     3150           
  Partials                          ?        0

mrbbot

Nice! Nearly there... 😅 Added some comments.

mrbbot · 2023-11-20T09:49:50Z

fixtures/dev-env/tests/index.test.ts


 type OptionalKeys<T, K extends keyof T> = Omit<T, K> & Partial<Pick<T, K>>;

 beforeEach(() => {
 	devEnv = new DevEnv();
 	mf = undefined;
-	res = undefined;
+	res = undefined as any;


Having | undefined in the res type rather than as any seems like a nicer solution here. Could we switch back to that and use runtime assertions/?. where needed?

packages/wrangler/src/api/startDevWorker/ProxyController.ts

packages/wrangler/templates/startDevWorker/InspectorProxyWorker.ts

packages/wrangler/src/dev/dev.tsx

packages/wrangler/templates/startDevWorker/InspectorProxyWorker.ts

packages/wrangler/templates/startDevWorker/ProxyWorker.ts

fixtures/worker-ts/src/index.ts

packages/wrangler/src/dev/local.tsx

packages/wrangler/src/dev/start-server.ts

packages/wrangler/src/api/dev.ts

fixtures/worker-ts/src/index.ts

…ker format

mainly, base innerUrl off of request.url not userWorkerUrl

instead of UserWorker ip/port

using the MF-Disable-Pretty-Error header on the UserWorker request the ProxyWorker will still interpret the json error response depending on its own MF-Disable-Pretty-Error header

by trying to start on a random port

by removing --runInBand flag for jest

+ remove miniflare log.error overrides no longer needed

not logger.error/info

Co-authored-by: MrBBot <[email protected]>

…time

* Revert "Revert "startDevWorker - Milestone 1" (#4171)" This reverts commit 88f15f6. * fix: don't show logs to ProxyWorker(s) unless log level is debug * fix: show console.log's in remote mode remote inspector websocket upgrade request required auth headers so use `fetch` with `Upgrade: websocket` header instead of `new WebSocket` * use miniflare verbose mode only if debug log level * use single Miniflare instance for (Inspector)ProxyWorker * port: clear remote runtime logs upon UserWorker restarts * default unstable_dev inspectorPort to 0 * parallelise cleanup to minimise chance of hanging previously, sequential cleanups fail to fully cleanup if earlier steps in the sequence fail * ensure InspectorProxyWorker unsafeDirectPort is set * don't use file-system for (Inspector)ProxyWorker DOs * prevent eviction of the Durable Objects with (Inspector)ProxyWorker * remove miniflare workaround for parallel requests * considerations for race between control messages and user fetches * use port: undefined vs 0 for UserWorker to force different port across reloads to workaround workerd bug on Windows * Don't try to parse `node-internal:* import specifiers * improve InspectorProxyWorker debug logs * only proxy consoleAPICalled events in remote mode * enable consoleAPICalled events proxying if local mode AND service-worker format * fix userWorkerInnerUrlOverrides host/hostname/port mainly, base innerUrl off of request.url not userWorkerUrl * use ProxyWorker ip/port for DEV_SERVER_READY event instead of UserWorker ip/port * always disable the UserWorker miniflare pretty error using the MF-Disable-Pretty-Error header on the UserWorker request the ProxyWorker will still interpret the json error response depending on its own MF-Disable-Pretty-Error header * recover from 'address in use' errors by trying to start on a random port * run unit tests in parallel again by removing --runInBand flag for jest * add handleRuntimeStdio option to ProxyWorker miniflare instance * expand containsHexStack check for windows * logger.debug runtime websocket errors from InspectorProxyWorker + remove miniflare log.error overrides no longer needed * log workerd warnings with logger.warn not logger.error/info * enable Cloudflare Access auth for remote previews * only send Runtime.discardConsoleEntries if currently connected to runtime --------- Co-authored-by: Samuel Macleod <[email protected]> Co-authored-by: MrBBot <[email protected]>

RamIdeas changed the base branch from main to startDevWorker-release November 9, 2023 13:50

mrbbot mentioned this pull request Nov 10, 2023

🐛 BUG: 3.15.0 breaks Remix HMR with "TypeError: error loading dynamically imported module" #4318

Closed

RamIdeas marked this pull request as ready for review November 13, 2023 18:08

RamIdeas requested a review from a team as a code owner November 13, 2023 18:08

RamIdeas force-pushed the startDevWorker-release branch from 261f092 to e708f82 Compare November 14, 2023 14:36

RamIdeas force-pushed the startDevWorker-milestone-1 branch 3 times, most recently from 8e2f1ce to 8b3c9b2 Compare November 15, 2023 02:43

mrbbot mentioned this pull request Nov 16, 2023

🐛 BUG: wrangler dev -> fetch failed #3121

Closed

RamIdeas force-pushed the startDevWorker-milestone-1 branch from 1348f52 to ee9ac52 Compare November 16, 2023 15:15

RamIdeas added the e2e Run wrangler e2e tests on a PR label Nov 16, 2023

mrbbot reviewed Nov 20, 2023

View reviewed changes

RamIdeas force-pushed the startDevWorker-release branch from e708f82 to c569378 Compare November 20, 2023 16:44

RamIdeas force-pushed the startDevWorker-milestone-1 branch from d6cad6b to b34416f Compare November 20, 2023 16:47

penalosa reviewed Nov 20, 2023

View reviewed changes

fixtures/worker-ts/src/index.ts Outdated Show resolved Hide resolved

packages/wrangler/src/dev/local.tsx Show resolved Hide resolved

packages/wrangler/src/dev/start-server.ts Show resolved Hide resolved

1000hz approved these changes Nov 21, 2023

View reviewed changes

packages/wrangler/src/api/dev.ts Show resolved Hide resolved

lrapoport-cf mentioned this pull request Nov 21, 2023

Remove React and Ink from wrangler dev #2714

Closed

RamIdeas force-pushed the startDevWorker-milestone-1 branch from 874b413 to 78c0b22 Compare November 21, 2023 17:10

mrbbot reviewed Nov 21, 2023

View reviewed changes

fixtures/worker-ts/src/index.ts Outdated Show resolved Hide resolved

RamIdeas force-pushed the startDevWorker-milestone-1 branch from 719e4f9 to 9b2ba1b Compare November 21, 2023 18:47

RamIdeas requested a review from a team as a code owner November 21, 2023 18:47

RamIdeas force-pushed the startDevWorker-milestone-1 branch from 9b2ba1b to 2931c48 Compare November 21, 2023 18:49

RamIdeas closed this Nov 21, 2023

RamIdeas reopened this Nov 21, 2023

RamIdeas force-pushed the startDevWorker-release branch from c569378 to e875506 Compare November 21, 2023 18:51

This was referenced Nov 22, 2023

startDevWorker Milestone 1 - Reboot #4175

Closed

🐛 BUG: EBUSY: resource busy or locked, rmdir #4167

Closed

RamIdeas and others added 19 commits November 22, 2023 20:02

fix linting

b05fffb

only proxy consoleAPICalled events in remote mode

fcb98b0

add changeset

55fcba2

enable consoleAPICalled events proxying if local mode AND service-wor…

c2e0670

…ker format

fix userWorkerInnerUrlOverrides host/hostname/port

9ea530c

mainly, base innerUrl off of request.url not userWorkerUrl

use ProxyWorker ip/port for DEV_SERVER_READY event

c84179b

instead of UserWorker ip/port

always disable the UserWorker miniflare pretty error

d82b949

using the MF-Disable-Pretty-Error header on the UserWorker request the ProxyWorker will still interpret the json error response depending on its own MF-Disable-Pretty-Error header

wip: recover from 'address in use' errors

7929c62

by trying to start on a random port

wip: try running unit tests in parallel again

691530a

by removing --runInBand flag for jest

ProxyController clean up

88c1993

add comments

85af8f7

add props.localUpstream to useEffect deps

e771632

undo unintended change

7f48c69

add handleRuntimeStdio option to ProxyWorker miniflare instance

8ec5f4f

expand containsHexStack check for windows

163ca3a

logger.debug runtime websocket errors from InspectorProxyWorker

2edcc05

+ remove miniflare log.error overrides no longer needed

log workerd warnings with logger.warn

52c082c

not logger.error/info

Remove unintended fixture changes

19efc4b

Co-authored-by: MrBBot <[email protected]>

enable Cloudflare Access auth for remote previews

e82dbcd

RamIdeas force-pushed the startDevWorker-milestone-1 branch from 2931c48 to e82dbcd Compare November 22, 2023 20:03

only send Runtime.discardConsoleEntries if currently connected to run…

7506c8f

…time

mrbbot approved these changes Nov 23, 2023

View reviewed changes

RamIdeas merged commit 29d7a4f into startDevWorker-release Nov 23, 2023

RamIdeas deleted the startDevWorker-milestone-1 branch November 23, 2023 14:09

RamIdeas mentioned this pull request Nov 23, 2023

startDevWorker - Milestone 1 #4497

Merged

6 tasks

admah mentioned this pull request Jan 9, 2024

🚀 Stabilizing unstable_dev and expanding Wrangler as an API #4730

Closed

admah added this to the Wrangler as a Library milestone Jan 9, 2024

lrapoport-cf linked an issue Jan 10, 2024 that may be closed by this pull request

🚀 Stabilizing unstable_dev and expanding Wrangler as an API #4730

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

startDevWorker Milestone 1 - Reboot #4413

startDevWorker Milestone 1 - Reboot #4413

RamIdeas commented Nov 9, 2023 •

edited

Loading

changeset-bot bot commented Nov 9, 2023 •

edited

Loading

github-actions bot commented Nov 9, 2023 •

edited

Loading

codecov bot commented Nov 16, 2023 •

edited

Loading

mrbbot left a comment

mrbbot Nov 20, 2023

startDevWorker Milestone 1 - Reboot #4413

startDevWorker Milestone 1 - Reboot #4413

Conversation

RamIdeas commented Nov 9, 2023 • edited Loading

changeset-bot bot commented Nov 9, 2023 • edited Loading

🦋 Changeset detected

github-actions bot commented Nov 9, 2023 • edited Loading

codecov bot commented Nov 16, 2023 • edited Loading

Codecov Report

mrbbot left a comment

Choose a reason for hiding this comment

mrbbot Nov 20, 2023

Choose a reason for hiding this comment

RamIdeas commented Nov 9, 2023 •

edited

Loading

changeset-bot bot commented Nov 9, 2023 •

edited

Loading

github-actions bot commented Nov 9, 2023 •

edited

Loading

codecov bot commented Nov 16, 2023 •

edited

Loading