-
Notifications
You must be signed in to change notification settings - Fork 30
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
Astro Env #894
base: main
Are you sure you want to change the base?
Astro Env #894
Conversation
Co-authored-by: Matthew Phillips <[email protected]>
Co-authored-by: Nate Moore <[email protected]>
I agree with @bholmesdev that we should align the APIs. I think I tend to prefer making public/private/client/server properties of the options object rather than chaining. This mostly comes down to the discussion above about what order chaining should happen. In a fluent API (another name for chaining) the expectation is that they can be chained in any order as they are operating on a common If we go with an options object we can make the types work by making them be a discriminated union where |
Some questions I have on the
I think I lean towards making at least one of these be required rather than picking a default, because accidentally bringing in values into the client, even if "public" would be a footgun. |
On the topic of |
I don't think we should strictly follow this. Instead I think we should follow a zod-like API so it feels unified. Zod also has certain cases where certain chains can only be used after another, for example Also, Zod supports .optional() and .default() instead of EDIT: The fluent API wiki also doesn't explicitly mention that methods need to / can be in any order. |
No comments from me, looks neat. I like the builder API for the |
I think we should have the fluent vs. options API discussion externally. It will probably block this RFC from being fully accepted but shouldn't affect implementation very much. I'll start a thread elsewhere. |
Hmmm... one thing that is not quite clear to me is how you would define an env variable for both client and server. Something like |
I agree it's not super clear but a client variable can be imported on both client and server |
Oh I see! Otherwise looks really nice! |
I've updated the RFC to match the current implementation PR state and make things clearer |
@florian-lefebvre thank you for this RFC. I think you mostly covered everything. However, something important that is missing is how an adapter gets to provide their "getter" function: |
proposals/0046-astro-env.md
Outdated
overrideProcessEnv({ | ||
getEnv, | ||
variables: [ | ||
{ | ||
destKey: 'PUBLIC_NETLIFY', | ||
srcKey: 'NETLIFY', | ||
}, | ||
{ | ||
destKey: 'PUBLIC_DEPLOY_URL', | ||
srcKey: 'DEPLOY_URL', | ||
default: url, | ||
}, | ||
], | ||
}); | ||
} |
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.
I don't think an adapter developer should import overrideProcessEnv
like this (from the runtime, using a long import). Instead, I think we should provide this function via hook, maybe in astro:config:done
(or another hook before the build). As you explained in the RFC, this is required during the dev/build, and we want to have these environment variables available before generating and rendering an actual page.
This way, we can control it, and we will provide this function only if the feature is enabled by the user and supported by the adapter. Win-win.
We could call it setOverrideProcessEnv
.
Also, can you explain what default
, destKey
, and srcKey
are? I looked at the source code and adapter PR, and I couldn't understand them.
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.
Yeah my bad I just wanted to get everything out of my head so some things related to the adapter may be messy!
About exposing it in a hook, i see a few issues:
- It can be called in quite a few hooks, from
astro:config:setup
toastro:build:setup
- It does nothing that involves astro (eg. settings)
So I don't know it it should be accessible from all those hooks parameters.
About the namings I'm happy to change them, just wanted something quick that worked to test on the adapters PR but basically:
destKey
is the key that should be set onprocess.env
srcKey
is the key passed togetEnv
inside the function. For example on netlify, that's important becauseNETLIFY
is exposed but it has to bePUBLIC_NETLIFY
with Astro Env, so it helps reducing boilerplatedefault
is meant to be used as a fallback value, for example forDEPLOY_URL
in dev
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 can be called in quite a few hooks, from
astro:config:setup
toastro:build:setup
Maybe I worded my thoughts badly, sorry! I meant, let's choose only one hook. If it's required for dev too, maybe astro:config:setup
makes more sense.
- It does nothing that involves astro (eg. settings)
Why? It sets an internal runtime function that is used inside the rendering phase (components that can read the environment variables)
destKey
is the key that should be set onprocess.env
srcKey
is the key passed togetEnv
inside the function. For example on netlify, that's important becauseNETLIFY
is exposed but it has to bePUBLIC_NETLIFY
with Astro Env, so it helps reducing boilerplate
default
is meant to be used as a fallback value, for example forDEPLOY_URL
in dev
Awesome! I am not very good at naming, so I am happy with the one chosen. It would be great if we could document them
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.
Why? It sets an internal runtime function that is used inside the rendering phase (components that can read the environment variables)
It's not! It's just setting process.env
so it doesn't call any internal astro stuff.
Maybe I worded my thoughts badly, sorry! I meant, let's choose only one hook. If it's required for dev too, maybe astro:config:setup makes more sense.
Well it's not exactly true. Yes astro:config:setup
makes sense for dev but I would also expose it another hook that runs in both dev and build (probably astro:config:done
) to allow the netlify build env variable inlining
Awesome! I am not very good at naming, so I am happy with the one chosen. It would be great if we could document them
Yes I'll add some
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.
If all this does is set a variable on process.env
then why do we need an API for this at all? Why don't they just set the variable themselves.
"astro:config:done": () => {
process.env.PUBLIC_NETLIFY = process.env.NETLIFY;
}
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.
That's what I started to do but there are a few issues that will be shared any adapters and so I thought a utility would help. For example, if NETLIFY
is undefined here, PUBLIC_NETLIFY
will actually be "undefined"
.
I just created this utility as the logic is a bit annoying, can be used several times per adapter (eg. dev and build) and can lead to errors easily.
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.
Given this is a large enough RFC already, I think adding this extra complexity (and maintenance) is not worth it at this time. I don't think this adds enough value to justify the maintenance in Astro. Integrations are advanced usage so they should be able to manage conditionally setting up environment variables in dev, if needed.
proposals/0046-astro-env.md
Outdated
|
||
##### `app.setGetEnv` | ||
|
||
Under the hood, `getSecret` uses `getEnv` from the runtime. It can be overrided by calling `app.setGetEnv`: |
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.
Why make this part of the App
class? The App
class is for rendering. Setting environment variables is unrelated to rendering for my hosts.
In addition, doing it this way means that variables can only be accessed inside of a render function. If you try to access a variable in the top-level of a module like so:
import { getSecret } from 'astro:env/server';
const API_URL = getSecret('API_URL');
This will result in the variable being undefined
because setGetEnv
is not called until the very end of the bundle.
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.
Initially I added getEnv
to app.render
options to have it per request but then moved it to a method. I thought calling app.setGetEnv
right after the instantiation of App
would be enough, where should that go then?
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.
I suppose we don't need the function app.setGetEnv
, and just expose the function setGetEnv
from astro/env
.
Users can still call setGetEnv
from anywhere inside their code, even during the rendering phase, and at the top level module.
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.
I suppose we don't need the function app.setGetEnv, and just expose the function setGetEnv from astro/env.
Alright so we can't use astro/env
as it's already taken by a dts (https://github.com/withastro/astro/blob/main/packages/astro/package.json#L34), what export should we use then?
Users can still call setGetEnv from anywhere inside their code, even during the rendering phase, and at the top level module.
As for matthew's example, it will still fail for Cloudflare I believe but I guess it's just a Cloudflare limitation in general.
Users can still call setGetEnv from anywhere inside their code, even during the rendering phase, and at the top level module.
Currently, I'm calling setGetEnv
with an implementation that throws (to fail if an adapter does not support getEnv
) in the App
constructor as I assumed it would run before anything else. Where should I put it instead?
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.
Yes, top-level usage is probably not possible in Cloudflare. That's just a limitation they've chosen, and shouldn't affect other runtimes.
As for where this lives, since adapters are built through Vite, is there any reason they can't just use a astro:env
export? astro:env/setup
or something.
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.
Sounds good
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.
The only issue I see is typing it, I guess it should not be done through typegen?
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.
I'm pretty sure you can add types for this, we have them in packages/astro/client.d.ts
Hello fellow Astronauts! We over @dmno-dev have been very deep in this specific problem space for the last few months, building out a platform-agnostic configuration tool meant to be a unified system for your entire stack. You can read more at dmno.dev and specifically check out our Astro integration. While our solution may be overkill for some folks, we hope it becomes the go-to solution for anyone working in a monorepo or wanting some tools with a bit more than what is built-in. Regardless, we are super happy to see the progress on this RFC and for Astro to have amazing first-class env var handling. Apologies for being a little late to the party, as I know some excellent implementation work by @florian-lefebvre is already underway, but I wanted to share a few thoughts here (that I’ve shared with him already) from what we've learned while building out DMNO. Please understand this is all meant to be constructive, simply to provoke discussion, and get the best possible outcome for Astro users. Challenges of hybrid renderingIn any SSR app (output This presents challenges on two fronts - preventing leaks, and reasoning about if config will be actually dynamic (vary at boot time) or not. There just is no easy way to enforce that the script part of a component has access to secrets, but only when being run on the server... Or to ensure that a config item you want to vary dynamically at boot time was not used during a pre-render of a page, effectively making it static even though it was not "bundled" into the code. When the line between client and server can be so blurry, I think it will lead to confusion to say an item is a “server” or “client” item, or vary behaviour depending on how it is accessed. Instead I think we should rely on a clear config schema to express the user's intent, build the system to do the right thing whenever possible, and to protect the user from anything unexpected. Config SchemaWhen we think about how to classify our config items, to me it really boils down to 2 concepts:
In DMNO this is accomplished by marking items with It should be noted that most users probably won't care about this dynamic/static stuff, and the default behaviour can still be set to The Edge cases
You can read more about DMNO's handling of this in our dynamic config guide. Access patternsThe current proposal has multiple methods of accessing certain config items which is supposed to help achieve safety. But this means you must think about which access pattern to use, and changing the schema of an item may necessitate changing how it is accessed throughout your code. By using a combination of proxy objects and build-time rewrites, the user gets to access everything exactly the same way, and it all just works as intended. In DMNO, we inject 2 globals (which could of course be imported from virtual modules instead)
NOTE - while the extra PUBLIC object is not strictly necessary, I do think it's helpful when browsing what items can be safely used on the client, and it's a bit closer to what folks are used to with the The injected proxy objects mostly let us give the user more helpful error messages (ex: “that item doesn’t exist” vs “that item exists but it is sensitive”), and it handles loading of any dynamic-public items. Loading these dynamic config items in the client uses a blocking http request so that we can maintain the same DX, rather than needing to do something like Protection from footgunsOnce we have a full schema to express the user's intent, we can effectively protect the user from doing the wrong thing, without having to think too much:
|
In doing a larger review of other tools, it looks like sveltkit is thinking along very similar lines to what I have proposed above - see https://kit.svelte.dev/docs/modules#$env-dynamic-private They use the prefix only as the marker of public/private, and the import method to help control static/dynamic behaviour. |
Yep we looked at it during the stage 2 rfc but thanks for bringing it again! |
Summary
astro:env
- Improve DX and security around environment variables in AstroLinks