-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Expose environment variables also on import.meta.env #8928
Conversation
Vite's built-in support for replacing environment variables in index.html has some issues. Especially with the way we handle environment variables. This works around those issues by implementing the replacement ourselves as a Vite plugin See also #8928
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.
Working locally for me. @Tobbe it doesn't see like we need the EnvironmentPlugin at all now with these changes, so I'd say we get rid of that just so we don't have duplicate code:
redwood/packages/vite/src/index.ts
Lines 223 to 236 in 3b548f2
// Loading Environment Variables, to process.env in the browser | |
// This maintains compatibility with Webpack. We can choose to switch to import.meta.env at a later stage | |
EnvironmentPlugin('all', { prefix: 'REDWOOD_ENV_', loadEnvFiles: false }), | |
EnvironmentPlugin( | |
Object.fromEntries( | |
rwConfig.web.includeEnvironmentVariables.map((envName) => [ | |
envName, | |
JSON.stringify(process.env[envName]), | |
]) | |
), | |
{ | |
loadEnvFiles: false, // to prevent vite from loading .env files | |
} | |
), |
Otherwise looks good to me!
@@ -29,6 +28,45 @@ export default function redwoodPluginVite(): PluginOption[] { | |||
const relativeEntryPath = path.relative(rwPaths.web.src, clientEntryPath) | |||
|
|||
return [ | |||
{ |
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.
Moved this plugin up here because it has order: 'pre'
which will make it run firsts. So intuitively it makes sense to me to have it first in the array of plugins. The reason it was further down before was because we wanted to group it with the other env-related plugin. But now that we removed that one that argument doesn't hold true anymore
Vite's built-in support for replacing environment variables in index.html has some issues. Especially with the way we handle environment variables. This works around those issues by implementing the replacement ourselves as a Vite plugin See also #8928
For compatability with webpack, and for consistency with node/server environment we want to support reading env vars from `process.env`. But we also want to support Vite's way of doing it, which is exposing them on `import.meta.env`. And this is a copy/paste from the code ``` // Vite can automatically expose environment variables, but we // disable that in `buildFeServer.ts` by setting `envFile: false` // because we want to use our own logic for loading .env, // .env.defaults, etc // The two object spreads below will expose all environment // variables listed in redwood.toml and all environment variables // prefixed with REDWOOD_ENV_ ``` Note: This does not work for Vite's html `%%` replacement because of vitejs/vite#13424 But we've got a separate PR that takes care of that in #8929
For compatability with webpack, and for consistency with node/server environment we want to support reading env vars from
process.env
. But we also want to support Vite's way of doing it, which is exposing them onimport.meta.env
.And this is a copy/paste from the code
Note: This does not work for Vite's html
%%
replacement because of vitejs/vite#13424But we've got a separate PR that takes care of that in #8929