-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add support for child environments #11879
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: f09f21f The changes in this PR will be included in the next version bump. 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
@cloudflare/workers-utils
wrangler
commit: |
1248067 to
8354d48
Compare
8354d48 to
f09f21f
Compare
|
Just for fun I tried out the new PR review feature in Devin: https://app.devin.ai/review/cloudflare/workers-sdk/pull/11879 |
| if (entryWorkerChildEnvironments) { | ||
| environmentNameToChildEnvironmentNamesMap.set( | ||
| entryWorkerEnvironmentName, | ||
| entryWorkerChildEnvironments | ||
| ); |
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.
🟡 Missing validation for child environment name collisions
In plugin-config.ts, there is validation that parent worker environment names don't collide with each other (line 380-383), but there is no validation that:
- A child environment name doesn't collide with a parent environment name
- A child environment name doesn't collide with another child environment name (from same or different parent)
- A child environment name doesn't collide with "client" (the reserved environment name)
In getEnvironmentsConfig (config.ts:155-197), child environments are added to workerEnvironments via Object.fromEntries. If there's a collision, the later entry silently overwrites the earlier one.
For example, if a user configures:
cloudflare({
viteEnvironment: {
name: "parent",
childEnvironments: ["parent"], // Overwrites parent!
}
})Or:
cloudflare({
viteEnvironment: {
name: "parent",
childEnvironments: ["client"], // Gets overwritten by client config!
}
})This would lead to silent misconfiguration and unexpected runtime behavior.
Recommendation: Add validation after setting child environments to ensure no child environment name collides with any parent environment name, other child environment names, or the reserved "client" name. This could be done by collecting all environment names and checking for duplicates before returning the resolved config.
Was this helpful? React with 👍 or 👎 to provide feedback.
petebacondarwin
left a comment
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 late here and my brain is not working at 100%.
But I read through all the code and it makes sense to me.
Moreover the playground tests appear to pass!
I assume you have tried this out with a real RSC project outside of the PR?
Devin picked up on a potential validation improvement but I don't think that should block my approval.
Nice work @jamesopstad.
Thanks for the review @petebacondarwin! The Devin comment is valid but I'll add the validation in a follow up PR so that it doesn't block things now. I've confirmed this works in real RSC projects using |
Add support for child environments.
This is to support React Server Components via @vitejs/plugin-rsc and frameworks that build on top of it. A
childEnvironmentsoption is now added to the plugin config to enable using multiple environments within a single Worker. The parent environment can import modules from a child environment in order to access a separate module graph. For a typical RSC use case, the plugin might be configured as in the following example:Notes
This is implemented to support the convention defined in vitejs/vite-plugin-react#1037. I have manually tested that it works with
@vitejs/plugin-rsc.I have omitted adding built in build functionality for child environments as it should be handled by another plugin e.g.
@vitejs/plugin-rscor the framework.A picture of a cute animal (not mandatory, but encouraged)