Closed saolsen closed 1 year ago
This may be related to https://github.com/withastro/astro/issues/4416#issuecomment-1228234136
I have created a PR #5103 to address the access to runtime issue (for cloudflare). Is there a way to do a similar thing with Netlify too? Than we can generalize this a little more, since that should be a task for the adapter to handle.
It probably can't be generic as each runtime does this a bit differently.
@bluwy is there any control we have over when import.meta.env
compiles to? If so maybe there could be an option that the adapter controls for this.
It probably can't be generic as each runtime does this a bit differently.
Sorry I did not mean to generalize what the adapter is doing, I meant the access to the specific runtime additions. Like ˋAstro.getRuntime()ˋ and each adapter can add to that runtime via a ˋSymbolˋ or ˋAstro.setRuntime()ˋ or even context, since it is also request related? I will adjust the draft PR.
@bluwy is there any control we have over when
import.meta.env
compiles to? If so maybe there could be an option that the adapter controls for this.
Yeah it's currently being transformed to process.env.*
at https://github.com/withastro/astro/blob/ef0c5431631665c9f6648ee5daee65a3ebf8e9ee/packages/astro/src/vite-plugin-env/index.ts#L37
We could expose an adapter option to customize this. I think Cloudflare has a special case where functions env vars can only be accessed throught the env
parameter, but this doc says otherwise that it can be read globally.
Either ways I think it would still be a win.
@AirBorne04 we would need an RFC for a generic solution. Your current cloudflare-only PR I think is fine without one. That's a bit different from the environment variable problem being discussed here though.
@bluwy oh, I didn't realize it was us. In that case I concur that an option that adapters can implement is the way to go here.
Maybe build.envKey
with a signature of (key: string) => string
Rough usage example:
'astro:config:setup': ({ updateConfig, mode }) => {
if(mode === 'build') {
updateConfig({
build: {
envKey: (key: string) => `Deno.env.get("${key}")`
}
})
}
}
I think Cloudflare has a special case where functions env vars can only be accessed throught the
env
parameter, but this doc says otherwise that it can be read globally.
When deploying pages you need to access it via the function parameters. Only in the workers you can read them globally. Pages and workers are working very similar but unfortunately not exactly the same. I ran a quick test in pages and it outputs:
Attempted to access binding using global in modules.
[w:server] You must use the 2nd `env` parameter passed to exported handlers/Durable Object constructors, or `context.env` with Pages Functions.
so definitely functions parameter for the pages platform, for workers it is either function parameter or global.
@AirBorne04 we would need an RFC for a generic solution. Your current cloudflare-only PR I think is fine without one. That's a bit different from the environment variable problem being discussed here though.
@matthewp just wondered if it makes sense to combine the env
vars with the additional platform features or request contexts.
@AirBorne04 I was talking about this with @bluwy, would it be possible, make sense for the adapter to assign the Env
to a global so that it can be accessed via this envKey
idea? Adapter would do something like:
const handler = (request, env) => {
globalThis.WORKER_ENV = env;
// ...
}
And then in the hook:
'astro:config:setup': ({ updateConfig, mode }) => {
if(mode === 'build') {
updateConfig({
build: {
envKey: (key: string) => `globalThis.WORKER_ENV["${key}"]`
}
})
}
}
I know this is weird but I can't think of another way to wire up import.meta.env
(which is what you want).
Am I right that the env
is not different between requests?
@matthewp Yes it is possible to assign them like this to a global. I am not sure how the envKey
would be working just like this envKey("NETLIFY_SECRET")
? And that would work with runtime specified env vars?
I know this is weird but I can't think of another way to wire up
import.meta.env
(which is what you want).
Actually i am not a fan of implicit merging of env variables into import.meta.env
, cause it would be rather difficult to find the actual source of the definition. I would slightly adjust the naming to something like runtimeEnv(key: string)
to be more explicit where they are defined.
Am I right that the
env
is not different between requests?
I checked this and the env vars are static per deployment
@AirBorne04 no, if you notice, the envKey
returns a string. That string is what gets compiled in place of where import.meta.env
is used. So in this case it would be pulling from a global.
Thinking about it, it might need to be a little more complex than that. If someone tries to use it at the top level (outside of a request) then the env
will not exist yet and it would through. So that code would need to guard against the global not being defined with.
Very well noted about differentiating between runtime vs. build-time env vars. I agree it's confusing / not clear. But that's the existing behavior so I think we have to stick with that until a better way is figured out.
Ah ok, if that is pushing them into the import.meta.env
i think it would be fine to do it this way. I can put together an example and try it out for cloudflare. I am not so familiar with netlify yet, but its a good opportunity to look at it also 😀
For the guard i will also check what can be done there.
Thinking about how we should implement this, it looks like currently we support:
Public | Private | |
---|---|---|
Static | ✅ | ❌ |
Dynamic | ❌ | ✅ |
I don't see a usecase yet for the two ❌ and it seems to have worked well for us so far. I think we can go with envKey
method. For a guard, the envKey
should be flexible enough to orchestrate checking in runtime.
@AirBorne04 have you started working on this? I'm also planning to test some things out soon.
yes @bluwy, I did start exploring a bit, you can have a look at my code. So far i was looking into how to make it work and have not managed to get the vars into the import.meta.env
but that might be related to the cloudflare
runtime as well.
Thanks @AirBorne04. Looks like you got the implementation pretty close. We'd need to replace using envKey
at https://github.com/withastro/astro/blob/37d664e26262f8e1026a31dcd4fcb251097dd90c/packages/astro/src/vite-plugin-env/index.ts#L37 and it should be complete.
I'll start working on a PR based on your branch too and credit you as a co-author. Will also implement a similar fix for Deno environments.
Turns out supporting build.envKey
is not as simple as I thought 😅 There's a lot to consider to not break things, particularly https://github.com/withastro/astro/pull/3327.
It also looks like we can't support runtime env vars when someone does const { SECRET } = import.meta.env
as it's hard to compile that without parsing JS.
Currently working on this branch which simplifies env var replacement & perf, but it still has some failing test. Once that's resolved we can start implementing the cloudflare and deno env support.
@bluwy I did a little more digging and see now why there is an issue. I have compiled the import.meta.env.USER
with the cloudflare adapter (Env compilation is handled by the env plugin though) and the replacement in the [[path]].js
file was actually process.env.USER
which makes total sense when we build for NodeJS, but is not available in cloudflare / netlify.
What is possible though is to actually expand the env vars
in the build step.
Build | Runtime |
---|---|
Env Vars (Build system + User) | Env vars (User) |
- | Bindings to KV, Durable, R2, D1 |
So for cloudflare there would be the path of adding the env vars to the SHIM
like
const SHIM = `globalThis.process = {
argv: [],
env: ${JSON.stringify(process.env)},
};`;
This would cause all the env vars to be included in the build and bloat the file size a little (3Kb in my case) but since this is server side that should be fine. This would also eliminate the need for build.envKey
implementation. And the currently promoted define via vite.
I will put a PR together as this is minimal effort, this could also be fixed in the other adapters like netlify and deno.
@AirBorne04 I'm currently close to a PR too with https://github.com/withastro/astro/tree/env-key and I actually found build.envKey
not needed at all too. Currently by assigning the env
from cloudflare to the shimmed process.env
is enough to fix it, as I now saw other integrations doing the same too.
What version of
astro
are you using?1.5.0
Are you using an SSR adapter? If so, which one?
@astrojs/netlify/edge-functions
What package manager are you using?
npm
What operating system are you using?
Linux
Describe the Bug
Using
import.meta.env
with ssr and@astrojs/netlify/edge-functions
doesn't work. For example if you have some code that referencesimport.meta.env.FOO
. The generated code has calls toprocess.env.FOO
but to look up environment values in the netlify edge runtime it needs to callDeno.env.get("FOO")
.Link to Minimal Reproducible Example
https://stackblitz.com/edit/github-gubo61?file=src/pages/index.astro
Participation