fix(next): do not override process.env

[florian-lefebvre]

Changes

• Fixes a bug that prevents env from being refreshed because of astro:env (we currently override process.env)
• It's breaking for adapters, hence it's a major change against next

Testing

Should still pass. I'll make a preview release to test adapters

Docs

Changeset, will do docs later

[changeset-bot]

🦋 Changeset detected

Latest commit: 0bb259d

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]

This PR is blocked because it contains a major changeset. A reviewer will merge this at the next release if approved.

[bluwy]

Linking back to a Vite issue that also describes this problem that has recent activity: vitejs/vite#17689

[bluwy]

I just realized something, why does Cloudflare sets the env to process.env.* in dev? Shouldn't astro:env only handle reading from the current running nodejs astro dev or build process.env.*, and the .env files? Does Cloudflare also polyfills process.env.* in the prod runtime?

(Talking about cloudflare since it seems to be the only integration that causes us to need to expose an API)

[florian-lefebvre]

That's because cloudflare env is loaded using wrangler platform proxy, it does not come from process env. We still need to call loadEnv() to load stuff from .env

[bluwy]

But why does the cloudflare platform proxy interfere with astro:env or process.env.*? Is that something cloudflare also does during build time or during prod runtime?

Anyways, if cloudflare wants to emulate process.env.* from the platform proxy for some reason, then I think rightfully they can simply assign to process.env.*. loadEnv() within Astro would take that in account anyways, so maybe we don't need to expose another API.

[florian-lefebvre]

There's a good reason for that! That's because an adapter can't set getEnv() in dev/build. But yeah I guess cloudflare may still be able to override process.env

[bluwy]

I think I get it now, and I think the true fix for this is indeed to have the adapters assign directly to process.env.* instead. The gist in dev seems to be that getEnv() should take the values loaded by loadEnv(). The issue is that loadEnv() also takes the process.env in account for dotenv expanding and precedence, so if adapters wanted to participate in that, they will have to assign to process.env.*.

If we follow this PR and assign the adapter env to a global symbol, it wouldn't work anyways because the next loadEnv() call in our Vite plugin would override the env?

---

Fundamentally, I don't think we should have assigned it to a global symbol or process.env.* in the first place since cross plugin and runtime boundaries will make future multi environment support even harder, but that's probably a different thing to tackle in the future.

[florian-lefebvre]

Just to confirm I understand correctly: what we should do is just have an internal thing on globalThis, and adapters keep augmenting process.env. Is that right?

[bluwy]

Yes 👍