Add support for reading .env files as runtime vars in local development#9914
Add support for reading .env files as runtime vars in local development#9914petebacondarwin merged 42 commits intomainfrom
.env files as runtime vars in local development#9914Conversation
🦋 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
This reverts commit 4523114.
There was a problem hiding this comment.
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.
Why set this here when it's set in startDefaultServe()?
There was a problem hiding this comment.
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.
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.
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.
Is this needed when using watcher.once()?
There was a problem hiding this comment.
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.
Can you explain why await miniflare.ready is needed in these two places?
There was a problem hiding this comment.
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.
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