Skip to content

[wrangler] make sure that the ready-on message is printed after the local runtime is ready - #9910

Merged
dario-piotrowicz merged 1 commit into
mainfrom
dario/DEVX-1978/read-on-after-containers-ready
Jul 23, 2025
Merged

[wrangler] make sure that the ready-on message is printed after the local runtime is ready#9910
dario-piotrowicz merged 1 commit into
mainfrom
dario/DEVX-1978/read-on-after-containers-ready

Conversation

@dario-piotrowicz

Copy link
Copy Markdown
Member

Fixes https://jira.cfdata.org/browse/DEVX-1978


  • Tests
    • Tests included
    • Tests not necessary because:
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: bugfix
  • Wrangler V3 Backport
    • Wrangler PR:
    • Not necessary because: changes related to a non-v3 feature

@changeset-bot

changeset-bot Bot commented Jul 9, 2025

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: cb375fc502291edd3904e7acc20751c8d3f36377

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

This PR includes changesets to release 3 packages
Name Type
wrangler Patch
@cloudflare/vite-plugin Patch
@cloudflare/vitest-pool-workers Patch

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

@dario-piotrowicz
dario-piotrowicz force-pushed the dario/DEVX-2012/quit-containers-build-image branch 2 times, most recently from 9e7515d to 7d953c4 Compare July 10, 2025 13:00
Base automatically changed from dario/DEVX-2012/quit-containers-build-image to main July 10, 2025 15:04
@dario-piotrowicz
dario-piotrowicz force-pushed the dario/DEVX-1978/read-on-after-containers-ready branch from 014ba3e to 810d429 Compare July 10, 2025 15:09
@github-project-automation github-project-automation Bot moved this to Untriaged in workers-sdk Jul 10, 2025
@dario-piotrowicz
dario-piotrowicz marked this pull request as ready for review July 10, 2025 15:10
@dario-piotrowicz
dario-piotrowicz requested a review from a team as a code owner July 10, 2025 15:10
@pkg-pr-new

pkg-pr-new Bot commented Jul 10, 2025

Copy link
Copy Markdown
create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@9910

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@9910

miniflare

npm i https://pkg.pr.new/miniflare@9910

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@9910

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@9910

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@9910

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@9910

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@9910

wrangler

npm i https://pkg.pr.new/wrangler@9910

commit: 764ec1e

@dario-piotrowicz
dario-piotrowicz force-pushed the dario/DEVX-1978/read-on-after-containers-ready branch 10 times, most recently from f2f607d to 39e1408 Compare July 11, 2025 00:12
Comment thread packages/wrangler/src/dev.ts Outdated
@github-project-automation github-project-automation Bot moved this from Untriaged to Approved in workers-sdk Jul 11, 2025
@dario-piotrowicz
dario-piotrowicz force-pushed the dario/DEVX-1978/read-on-after-containers-ready branch 2 times, most recently from 534811f to c9275b3 Compare July 11, 2025 10:35
Comment thread packages/wrangler/src/dev.ts Outdated
@dario-piotrowicz
dario-piotrowicz force-pushed the dario/DEVX-1978/read-on-after-containers-ready branch 2 times, most recently from b4769ba to 5a2dd18 Compare July 16, 2025 22:30

@emily-shen emily-shen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

so if i understand correctly, now we wait for all runtime controllers to be ready or torn down before printing ready - this might be a proxy server, the one with the actual user worker, or remote. is that right?

"--port=0",
"--inspector-port=0",
]);
vi.waitFor(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why are there changes to a pages fixture?

@dario-piotrowicz dario-piotrowicz Jul 17, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The test below does the following:

  • clears the output (clearOutput())
  • renames a file
  • makes sure that the output is (still) empty (expect(getOutput()).toBe(""))

Before ready on was being printed basically at the earlier time (as soon as the proxy controller is initialized), before the output gets cleared, but now the log is being printed asynchronously when all runtimes are ready, which actually happens after the output gets cleared (making the toBe("") assertion fail).
That's why now we need to wait for the ready on message to be there first (so that we can be sure that that won't appear in the output)

If you'd prefer a different change here please let me know 🙂

Comment thread packages/wrangler/src/api/startDevWorker/DevEnv.ts Outdated
@dario-piotrowicz

Copy link
Copy Markdown
Member Author

so if i understand correctly, now we wait for all runtime controllers to be ready or torn down before printing ready - this might be a proxy server, the one with the actual user worker, or remote. is that right?

Yes 🙂

Comment thread packages/wrangler/src/api/startDevWorker/DevEnv.ts Outdated
Comment thread packages/wrangler/src/api/startDevWorker/DevEnv.ts Outdated
Comment thread packages/wrangler/src/api/startDevWorker/DevEnv.ts Outdated
Comment thread packages/wrangler/src/api/startDevWorker/BaseController.ts Outdated
Comment thread packages/wrangler/src/dev.ts Outdated
@dario-piotrowicz
dario-piotrowicz force-pushed the dario/DEVX-1978/read-on-after-containers-ready branch from d92420b to cb375fc Compare July 22, 2025 16:21
Comment thread packages/wrangler/src/api/startDevWorker/DevEnv.ts Outdated
@dario-piotrowicz
dario-piotrowicz force-pushed the dario/DEVX-1978/read-on-after-containers-ready branch from cb375fc to 764ec1e Compare July 23, 2025 14:27

@penalosa penalosa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great! Thanks for putting up with all my nitpicking here

@dario-piotrowicz

Copy link
Copy Markdown
Member Author

Looks great! Thanks for putting up with all my nitpicking here

No problem, thanks a lot for correcting all my silly mistakes! 🙏

@dario-piotrowicz
dario-piotrowicz merged commit 7245101 into main Jul 23, 2025
33 checks passed
@dario-piotrowicz
dario-piotrowicz deleted the dario/DEVX-1978/read-on-after-containers-ready branch July 23, 2025 16:10
@github-project-automation github-project-automation Bot moved this from Approved to Done in workers-sdk Jul 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants