Remove default throw behavior in astro:env

withastro / astro

The web framework for content-driven websites. ⭐️ Star to support our work!

https://astro.build

Other

46.93k stars 2.49k forks source link

Remove default throw behavior in astro:env #12480

Closed matthewp closed 2 days ago

matthewp commented 2 days ago

Changes

Removes the default throw for getEnv.
Plan is to move away from having astro:env be dependent on timing; if some code runs before setGetEnv has been called it currently could result in incorrect or missing environment variables.
Instead now we always default to the process.env[key] approach. Since this is what most adapters need anyways, we don't have to worry about bundling causing issues of code executing in unwanted order.
Now only Cloudflare needs to ensure it calls setGetEnv, and it already has the constraint of not allowing vars to be grabbed at the top level.

Testing

manual

Docs

N/A, bug fix

changeset-bot[bot] commented 2 days ago

🦋 Changeset detected

Latest commit: 2ae06b195e618c368cce3090a874908bfa86a5e0

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

codspeed-hq[bot] commented 2 days ago

CodSpeed Performance Report

Merging #12480 will not alter performance

_{Comparing env-no-throw (2ae06b1) with main (46f6b38)}

Summary

✅ 6 untouched benchmarks

florian-lefebvre commented 2 days ago

More stuff can be removed:

the referenced error
no need to pass astro:env data to the ssr manifest I believe
requires updating the base pipeline arguments (see https://github.com/withastro/astro/commit/80edd817b90a233dafd8bbb3fa0f38e7a8ff09a0#diff-faa73d40645eda14a766bb44509025ac3a3758731ae7f63d657b3cd230961e9f and https://github.com/withastro/astro/commit/80edd817b90a233dafd8bbb3fa0f38e7a8ff09a0#diff-0a19d4e7d5ce97887629902a09e7ecb9f4c5591c15ebcec95c28ceffc61cc3f7L59)

matthewp commented 2 days ago

@florian-lefebvre made the changes you suggested. We're still doing validation though, @ematipico is going to work on another approach that doesn't rely on setGetEnv.