Skip to content

Conversation

@petebacondarwin
Copy link
Contributor

@petebacondarwin petebacondarwin commented Jul 10, 2025

Fixes #9394

TODO


@changeset-bot
Copy link

changeset-bot bot commented Jul 10, 2025

🦋 Changeset detected

Latest commit: 7ec7a0c

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

This PR includes changesets to release 3 packages
Name Type
@cloudflare/vite-plugin Major
wrangler Minor
@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

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jul 10, 2025

create-cloudflare

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

@cloudflare/kv-asset-handler

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

miniflare

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

@cloudflare/pages-shared

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

@cloudflare/unenv-preset

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

@cloudflare/vite-plugin

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

@cloudflare/vitest-pool-workers

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

@cloudflare/workers-editor-shared

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

wrangler

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

commit: 7ec7a0c

@petebacondarwin petebacondarwin force-pushed the pbd/wrangler/dotenv branch 3 times, most recently from 9efb7bb to 9c3ff21 Compare July 10, 2025 16:32
@petebacondarwin petebacondarwin force-pushed the pbd/wrangler/dotenv branch 16 times, most recently from 6cbaf87 to b816f7a Compare July 17, 2025 13:54
When step-through debugging you can often get stuck on this bit of code,
where there is an unhandled rejection in the body reading code.
It is not apparent in real life and has no effect outside of debugging.
- use mockfile in config-changes test
- each test in this file expects a starting condition but needs to wait for Vite to be ready since it might be rebooting.
- only listen for one config change before restarting
- abort extra restarts
Copy link
Contributor

@jamesopstad jamesopstad left a comment

Choose a reason for hiding this comment

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

Great work persevering Pete!

A couple of general comments in addition to those inline:

  • A lot more of the tests now wrap expect in waitUntil. As I understand it each test file should now get its own Miniflare instance so this shouldn't be necessary unless the tests cause the dev server to restart. Is that incorrect?
  • There is now a requirement for tests that use page to call page.goto(viteTestUrl) manually at the start of each test. I think that's OK but just something we'll have to watch out for as tests in the same file now continue from the previous test's state.
  • If we're going to start including debug logs then I think we could do this more generally and use specifiers like Vite does. Not for this PR but might be nice to follow up using the approach here.
  • Generally, I'm most concerned about how much code has been added and that it makes reading the source more challenging. If there's any room for now stripping back some of the changes that have been made then that would be good.
  • For posterity, it would be really helpful if you could add a comment to this PR summarising what the issues were and how they've been resolved.

}
} else {
await startDefaultServe();
server = await startDefaultServe();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why set this here when it's set in startDefaultServe()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I think at this stage I was being overly conservative since I wasn't sure whether the startDefaultServe() was mutating things correctly. Arguably it shouldn't mutate server at all.

Comment on lines +362 to +365
// It is possible to get into a situation where the dev server is restarted by a config file change
// right in the middle of the Vite server and the supporting Workers being initialized.
// We use an abort controller to signal to the initialization code that it should stop if the config has changed.
const restartAbortController = new AbortController();
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to establish if this is still necessary with all the other changes as it makes the code quite a bit more complex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we should rip this out if it is not needed

"Restarting dev server and aborting previous setup"
);
restartAbortController.abort();
await viteDevServer.watcher.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed when using watcher.once()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just realised that watcher.once() is incorrect, since it will be triggered by lots of other file changes.
I meant to revert that and just remove the watcher if we are reverting.
It happens to work in the tests because we don't touch any other files.

Comment on lines +447 to +453
debuglog(configId, "Waiting for Miniflare to be ready before update");
await miniflare.ready;
debuglog(configId, "Updating the Miniflare instance");
await miniflare.setOptions(miniflareDevOptions);
debuglog(configId, "Waiting for Miniflare to be ready after update");
await miniflare.ready;
debuglog(configId, "Miniflare is ready");
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why await miniflare.ready is needed in these two places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's probably not. I was just chucking stuff in there to see if waiting for things helps.

Comment on lines +367 to +368
// We use a `configId` to help debug how the config changes are triggering the restarts.
const configId = randomUUID();
Copy link
Contributor

Choose a reason for hiding this comment

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

Creating this value and passing it around has added a lot of code. Do we still need it now that you've resolved the issues?

@petebacondarwin
Copy link
Contributor Author

petebacondarwin commented Jul 30, 2025

Fixes attempted that should be checked and reverted if not necessary:

@petebacondarwin petebacondarwin merged commit a24c9d8 into main Jul 30, 2025
50 of 53 checks passed
@petebacondarwin petebacondarwin deleted the pbd/wrangler/dotenv branch July 30, 2025 12:21
@github-project-automation github-project-automation bot moved this from Approved to Done in workers-sdk Jul 30, 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