-
Notifications
You must be signed in to change notification settings - Fork 1k
Add support for reading .env files as runtime vars in local development
#9914
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
Conversation
🦋 Changeset detectedLatest commit: 7ec7a0c The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
9efb7bb to
9c3ff21
Compare
6cbaf87 to
b816f7a
Compare
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
2792327 to
7a94a56
Compare
This reverts commit 4523114.
7a94a56 to
c311775
Compare
There was a problem hiding this 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
expectinwaitUntil. As I understand it each test file should now get its ownMiniflareinstance 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
pageto callpage.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(); |
There was a problem hiding this comment.
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()?
There was a problem hiding this comment.
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.
| // 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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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()?
There was a problem hiding this comment.
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.
| 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"); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| // We use a `configId` to help debug how the config changes are triggering the restarts. | ||
| const configId = randomUUID(); |
There was a problem hiding this comment.
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?
Avoid the `server` variable being global and mutated
|
Fixes attempted that should be checked and reverted if not necessary:
|
Fixes #9394
TODO
docs changes:
escape hatch for i.e. Next (which load
.envfile using a different mechanism)added
CLOUDFLARE_LOAD_DEV_VARS_FROM_DOT_ENVopt-out env var