Use esbuild for env replacement #9652
Conversation
🦋 Changeset detectedLatest commit: fe16773 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 |
github-actions
bot
added
pkg: integration
| code: escapeViteEnvReferences(String(compiled.value)), | ||
| code: String(compiled.value), |
There was a problem hiding this comment.
We can safely remove this in a non breaking way as the plugin calls fs.readFile, bypassing Vite's transform. The only continuing env replacement left after this transform is Vite's define, which we know is fixed in Vite 5.
Luckily Astro's env replacements (which didn't work well in plain strings) happen before the fs.readFile, so the broken code was thrown away.
| @@ -29,7 +33,7 @@ function getPrivateEnv( | |||
| const privateEnv: Record<string, string> = {}; | |||
| for (const key in fullEnv) { | |||
| // Ignore public env var | |||
| if (envPrefixes.every((prefix) => !key.startsWith(prefix))) { | |||
| if (isValidIdentifierRe.test(key) && envPrefixes.every((prefix) => !key.startsWith(prefix))) { | |||
There was a problem hiding this comment.
Added a valid identifier check because keys could be ProgramFiles(x64) which wouldn't work as import.meta.env.ProgramFiles(x64). This worked before because we do plain string replacements, but I don't think people were relying on this to work.
| configResolved(resolvedConfig) { | ||
| viteConfig = resolvedConfig; | ||
|
|
||
| // HACK: move ourselves before Vite's define plugin to apply replacements at the right time (before Vite normal plugins) |
There was a problem hiding this comment.
Copied this hack from @astrojs/mdx.
| @@ -11,7 +11,7 @@ export function recmaInjectImportMetaEnv({ | |||
| if (node.type === 'MemberExpression') { | |||
| // attempt to get "import.meta.env" variable name | |||
| const envVarName = getImportMetaEnvVariableName(node); | |||
| if (typeof envVarName === 'string') { | |||
| if (typeof envVarName === 'string' && importMetaEnv[envVarName] != null) { | |||
There was a problem hiding this comment.
This check was the main reason for the error in #9012, since if it's undefined, we accidentally generate a broken AST.
After fixing this, it relies on other fixes in this PR so that it works correctly in runtime.
Changes
fixes #7503
fixes #9012
In dev, we can use a fast path to assign all private env vars to
import.meta.envon the server-side runtime, which is a lot faster and is what Vite does too.In build, we use esbuild to perform the replacements so that we don't accidentally replace env vars inside strings, same as Vite.
Testing
Existing tests should pass. I also added test for the MDX issue, but not for the Vue one. It's a bit harder to test that, but I can confirm it's fixed with the repro manually.
NOTE: The changes in
astroand@astrojs/mdxare mutually exclusive, which means the user can use the latest version ofastroand older version of@astrojs/mdxat the same time just fine. (and vice versa)Docs
n/a. Ideally this should be transparent to users.