feat: rfc api #4
Conversation
| Those variables can be accessed at runtime (if your host supports it). They can be accessed through `locals`. | ||
|
|
||
| ```ts | ||
| // src/pages/index.astro | ||
| Astro.locals.env.MY_DYNAMIC_VAR | ||
| ``` |
There was a problem hiding this comment.
Dynamic variables would be accessible through a virtual module as well. I know @ lilnasy suggested using something like
Astro.env.KEYbut I think the issue is that it makes it hard to use env variables outside of.astrofiles and endpoints
-- You 😄
Same argument applies to Astro.locals.env right?
I know we shouldn't use virtual imports with the astro: prefix, but is astro:env used currently? If it isn't and this is to implement the API as it would be in Astro maybe it should be imported from there.
But if we want to be safe it could be from astro-env:values or #astro-env;
There was a problem hiding this comment.
Well I initially wanted a virtual module for this but env relies on the current requests with some adapters like cloudflare so locals (in userland but Astro.env in core) looks like the best place for it. This is based on a convo on Discord with arsh and alex
| virtualImport: readFileSync( | ||
| resolve(`./stubs/dynamic-env/${runtime}.mjs`), | ||
| "utf-8", | ||
| ), |
There was a problem hiding this comment.
This seems problematic for projects with mixed output.
Values read from the runtime on pre-rendered pages will run on Node, not the adapter's runtime.
There was a problem hiding this comment.
Ah that's interesting! Actually this is a workaround but that will eventually be part of the adapter api. Does it change anything tho? I don't think so. @alexanderniebuhr I'm interested in your thoughts on this
There was a problem hiding this comment.
In theory I don't think it matters. The only thing which is important, IIUC, is that the variable is set and available in both (node & other runtime) and be named the same.
The only difference would be how the var is resolved, correct?
The only question is, does it work for runtimes without fs if you use readFileSync here?
There was a problem hiding this comment.
Here the usage of fs doesn't matter because it's inside the integration. In the core implementation, this piece of code will not exist anyway because adapters will provide the implementation as an entrypoint or something
There was a problem hiding this comment.
That is not the problematic part. The file will be loaded by Node since this run at build time. But if the project tries to read a dynamic variable on a prerendered page it will break.
When deploying to Cloudflare, for example, it will use the code that works on SSR on Cloudflare, but that code doesn't run on Node.
It will break on the route using it, not here
There was a problem hiding this comment.
Alright so the logic would be
- On build/prerender, use node env resolution
- In SSR, use the adapter logic if defined, otherwise fallback to node env resolution
Is there a way to know what is the current case and use that somehow?
There was a problem hiding this comment.
Is there a way to know what is the current case and use that somehow?
There is no easy way for it currently. You can do a runtime check of what runtime are you on between Node and the adapter's runtime by checking typeof process. This allows detecting whether it is running on Node in those cases.
There was a problem hiding this comment.
checking
typeof process
That wouldn't work in all cases.. I guess the safer way would be to use Astro's config to see which adapter is set 🤔
| }); | ||
| }, | ||
| "astro:server:setup": ({ server }) => { | ||
| // TODO: do not kill the terminal, show in the error overlay |
There was a problem hiding this comment.
I did a quick read over that logic in core, and it doesn't seem like an integration has access to it as it is today.
There was a problem hiding this comment.
thanks! That was suggested by erika or nate, i'll just throw then
| get(_target, prop, _receiver) { | ||
| if (typeof prop === "string") { | ||
| return getEnvVariable(prop, { locals }); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Writing to this proxy will be silently ignored.
> p = new Proxy({}, {get: (_, prop) => prop})
Proxy [ {}, { get: [Function: get] } ]
> p.foo
'foo'
> p.foo = 123
123
> p.foo
'foo'
I think it should either be a true write, allowing envs to be modified or it should throw an error.
| get(_target, prop, _receiver) { | |
| if (typeof prop === "string") { | |
| return getEnvVariable(prop, { locals }); | |
| } | |
| }, | |
| get(target, prop, _receiver) { | |
| if (typeof prop === "string") { | |
| return target[prop] ?? getEnvVariable(prop, { locals }); | |
| } | |
| }, |
There was a problem hiding this comment.
I'm not familiar with proxies at all tbh! Any improvement is more than welcome
runtimeoption would not be there in the core implementation. It would be something done by the adapters themselves