Skip to content
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

How should env variables be exposed? #8

Open
CanadaHonk opened this issue Feb 6, 2024 · 9 comments
Open

How should env variables be exposed? #8

CanadaHonk opened this issue Feb 6, 2024 · 9 comments
Labels

Comments

@CanadaHonk
Copy link
Member

A bare object (process.env, Bun.env)? A Map-like object (Deno.env)?

@CanadaHonk CanadaHonk added the spec label Feb 6, 2024
@CanadaHonk
Copy link
Member Author

@lucacasonato Do you (or another Deno member) know why Deno chose a Map-like object instead of just a bare object with getters/setters?

@lucacasonato
Copy link
Member

Because "getting all" and "getting one specific" require different permissions.

@CanadaHonk
Copy link
Member Author

Ah that makes sense. I didn't think of this before, but we should ensure the proposed API works with permission systems and situations with limited permissions generally.

@paperdave
Copy link

paperdave commented Feb 8, 2024

just putting this out there that environment variables on Windows are case-insensitive. node.js and bun implement process.env on windows as a case-insensitive object, which goes against the normal logic of what an object is. It is a Proxy!

for a full formal spec, i would probably want it to be an object that implements a Map interface, but is not explicitly a Map (it could extend map?), so that the get function can be defined to do the expected case normalization on platforms that have such requirements. the permissions thing also makes sense for this.

but maps take more thought to use, and are incompatible with "define plugin"s like bun and esbuild's --define, the define option in rollup, and the good old DefinePlugin from webpack; these expect global identifiers and property accesses, such as process.env.NODE_ENV, and are important for building web apps that need secrets or other values stored in environment variables. vite has import.meta.env which is their own thing that is some sugar over define plugin.

not really sure the best thing but these are some things to think about when it comes to mind.

edit: the webpack define plugin might allow functions; never have seen it used that way if that is the case.

@CanadaHonk
Copy link
Member Author

Another point on spec details: if called with a non-string should it be converted to one or always return undefined/etc?

@lucacasonato
Copy link
Member

I think it should throw (no implicit conversion).

@ljharb
Copy link
Member

ljharb commented Feb 8, 2024

Throwing would match TC39’s future language direction in that regard; it’s definitely the better choice.

@CanadaHonk
Copy link
Member Author

CanadaHonk commented Feb 8, 2024

I am currently leaning towards a Proxy-like approach where environment variables are only gotten when the getter is ran so it works with permission systems; but also easy/simple to access with bare properties. The potential issues I can think of so far:

  • How easy a Proxy-like object with getter/setter/etc for any key is to implement for native code (interfacing with JS engines)?
  • Should we add a custom [[OwnPropertyKeys]] as well to allow getting all environment variables set? It would allow for eg: for (const name in CLI.env) // ..., and Object.keys(CLI.env).
  • Could this confuse users by being too "magic" (not behaving like anything else JS)?

@CanadaHonk
Copy link
Member Author

I pushed an initial simplified ES-like spec in the readme for this, but I am very new to ES spec writing so it probably isn't great ;)

@CanadaHonk CanadaHonk added agenda+ Should be discussed next meeting and removed agenda+ Should be discussed next meeting labels Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants