-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Support environment variables in Cloudflare and Netlify Edge functions #5301
Conversation
🦋 Changeset detectedLatest commit: b30d076 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 |
hey @bluwy , i love it! CodeI think for the Cloudflare adapter there are multiple Is this still needed? astro/packages/integrations/cloudflare/src/shim.ts Lines 1 to 4 in 3976513
This is run at build time so it could be used to map all build env vars into the process env shim astro/packages/integrations/cloudflare/src/index.ts Lines 30 to 33 in 3976513
If it is changed to env: JSON.stringify(${process.env}) making the guard/proxy obsolete (this is not 100% the same as the request env vars though which can only be added later)
Having said that, i think it is fine either way (guard/proxy or build vars) probably the proxy is a bit more straight forward to understand for the user and creates less headaches for maintance. DocsFor the docs i think the Cloudflare adapter docs for Environment vars need to be adjusted. |
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! LGTM.
privateEnv.SITE = astroConfig.site ? `'${astroConfig.site}'` : 'undefined'; | ||
privateEnv.SSR = JSON.stringify(true); | ||
privateEnv.BASE_URL = astroConfig.base ? `'${astroConfig.base}'` : 'undefined'; |
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.
Greatly appreciate these refactors! I knew this plugin was messy and never had time to come back and clean it up. 🎉
{ | ||
get: (target, prop) => { | ||
console.warn( | ||
// NOTE: \0 prevents Vite replacement |
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.
WHAT! I totally missed this. That's very smart.
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 slightly leaning towards a hack that should be fixed in Vite 4 (hopefully) 😬
Nice find! Yeah we should clean them up.
Interesting fix, I wondered why you did that before. I'm not really sure about this yet as it brings up the bundle size by default, but we can revisit this idea if the Proxy is confusing for users.
Yes 👍 This should be updated too. Thanks for the review! |
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.
Thanks for this, @bluwy!
I've left a couple of comments for your consideration regarding the README. You can consider this reviewed by Team Docs now! 🥳
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Final LGTM approval from me! 🚀 |
#5301) Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Changes
Fix #5105
Fix #5234
vite-plugin-env
. Does lesser work and should be more performant.env
to globalprocess.env
.Note: From the linked issue, we discussed of the possibility of
build.envKey
, but I later found it's not needed as we can assign platform env vars to a shimmedprocess.env
and it should be enough. Currently the Deno integration does this so there's a precedence to follow. Also one less configuration 😬Note 2: Thanks @AirBorne04 for the discussion and ideas that reached here.
Testing
Existing env var test should pass. Added new tests for Cloudflare and Netlify Edge.
Cloudflare and Netlify Edge tests are previously skipped and I can't seem to get it passing for CI. However it was working when testing locally.
Docs
Updated docs in Cloudflare integration README.