| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
|
I think we'll get a number of follow up requests beyond the most basic functionality. A few that I think we should have some plan for:
Note, some of these are taken directly from the dotenv module's README. |
Sorry, something went wrong.
|
regarding testing and validation, you can take dotenv's test suite (which afaik is the most popular package for parsing .env files): https://github.com/motdotla/dotenv/tree/master/tests my other concerns were already mentioned just before I was about to comment |
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you for doing this! My vote is that the flag should be --env-file [path-to-file] so like --env-file .env or --env-file .env.development or whatever, so that people can have multiple files per environment like how Bun handles this. If we’re going to require a flag at all, we might as well make it versatile.
And a future PR can potentially add support for configuring Node without needing opt in via flag, like potentially a node.config.json file or a field in package.json; but those implementations can live alongside this one.
Sorry, something went wrong.
I think it’s okay if we define our scope as more limited than dotenv‘s, and if people want something more full-featured then they should just use dotenv. Ultimately the point of adding this is to fix #48852 and #43973, to just provide a way to configure Node, and the ability to set other environment variables beyond NODE_OPTIONS is a side benefit. That said, from your list I already mentioned allowing specifying the path to the file, and I think we need to at least document what we do for conflicts; I think the simplest approach is to just always have the “real” environment variable win out over whatever is defined in the file, and leave it at that. It would be nice to ignore lines starting with # as comments, as that should hopefully be pretty easy. The rest I think we could consider out of scope, at least for now. One other thing we should do is mark this as experimental in the docs. |
Sorry, something went wrong.
There was a problem hiding this comment.
There is a question: how about .env.local .env.*.local …
there will have multiple different env file during the development
Sorry, something went wrong.
|
Supporting incremental .env files would be great (e. .env.test with overrides over generic .env) |
Sorry, something went wrong.
|
See, the feature requests already start piling up... Apart from this being an infinitely slippery slope, I also feel it goes directly against node's "mechanism over policy" philosophy. Not a fan. |
Sorry, something went wrong.
|
Please consider adding support for env expand. It's insane to think that dotenv-expand has 12 million weekly download which has no defined grammer. test$var the list goes on. Too much regex in Node community 🙂 |
Sorry, something went wrong.
I think it can be implemented as a follow-up pull request, if anyone is interested in implementing. I'm in favor of keeping the implementation as small as possible (for now). |
Sorry, something went wrong.
|
What is this feature's interaction with the permission model? |
Sorry, something went wrong.
@panva If we want to support NODE_OPTIONS, it needs to bypass permission model, since V8 will not be initialized at that time. |
Sorry, something went wrong.
So an env variable that pretty much can override node options... That is honestly risky in many fronts even more when it boils down to bypassing permission mode options. |
Sorry, something went wrong.
|
@nodejs/tsc This seems like the kind of thing the TSC should have an opinion on. |
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM!
Amazing, thanks for this!
Sorry, something went wrong.
|
Thanks @anonrig! You've saved a few hundred thousand people (maybe more) a little work by including this! ❤️ |
Sorry, something went wrong.
| "[has_env_file_string]", "", &EnvironmentOptions::has_env_file_string); | ||
| AddOption("--env-file", | ||
| "set environment variables from supplied file", | ||
| &EnvironmentOptions::env_file); |
There was a problem hiding this comment.
@anonrig Was it a deliberate decision to not set kAllowedInEnvvar for this option?
I'm asking, because in order to replace the dotenv-npm-package with the native dotenv-package (this feature) AND using npm scripts, I need to set NODE_OPTIONS='--env-file .env' in my .npmrc file.
Currently, this is not possible because kAllowedInEnvvar is not set.
Sorry, something went wrong.
There was a problem hiding this comment.
It wasn't a deliberate decision. Prs are welcome :-)
Sorry, something went wrong.
There was a problem hiding this comment.
Is there a risk of a loop here? Like what if an .env file contains NODE_OPTIONS='--env-file .env'?
Sorry, something went wrong.
There was a problem hiding this comment.
No there isn't
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for the info! I'll read through the contribution guidelines and open a PR.
Sorry, something went wrong.
There was a problem hiding this comment.
it would be really useful when running node-scripts via npm
How so? What is a script you would use it for?
Sorry, something went wrong.
There was a problem hiding this comment.
How so? What is a script you would use it for?
In my case I'm running playwright via npm like this:
{
"name": "npm-package",
"scripts": {
"test:browser": "playwright test"
}
}Inside my playwright config, I'm currently using dotenv.config() to get the baseUrl for the current env.
import dotenv from 'dotenv';
dotenv.config();
const config: PlaywrightTestConfig = {
baseURL: process.env.PLAYWRIGHT_BASE_URL,
};
export default config;I'd like to get rid of the npm dotenv-dependency and use the native Dotenv functionality, without having to change my npm scripts to something like node --env-file=.env node_modules/.bin/playwright test.
Sorry, something went wrong.
There was a problem hiding this comment.
without having to change my npm scripts to something like node --env-file=.env node_modules/.bin/playwright test
So you're saying you prefer to do NODE_OPTIONS=--env-file=.env playwright test? Node doesn't automatically load .env files.
Sorry, something went wrong.
There was a problem hiding this comment.
So you're saying you prefer to do NODE_OPTIONS=--env-file=.env playwright test? Node doesn't automatically load .env files.
Yes, sort of.
As far as i know, I can use .npmrc and node-options to automatically apply NODE_OPTIONS to called lifecycle scripts (like playwright).
Sorry, something went wrong.
There was a problem hiding this comment.
As far as i know, I can use .npmrc and node-options to automatically apply NODE_OPTIONS to called lifecycle scripts (like playwright).
Interesting, I didn’t know about node-options!
Sorry, something went wrong.
|
Which is the standard or recommended file pattern? .env.foo or .foo.env? Corepack and Node official document prefer the later one. Refer to https://nodejs.org/en/learn/command-line/how-to-read-environment-variables-from-nodejs |
Sorry, something went wrong.
.foo.env is the preferred one |
Sorry, something went wrong.
|
A further suggestion: We should treat .env as a file extension. Therefore, disallowing the files with non .env extension for --env-file will benefit Node community. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Follow-up from the comments:
@cjihrig:
@KhafraDev
@GeoffreyBooth
Missing features
The following features are not implemented in this current context. And can be implemented in a separate PR.
Ref: https://github.com/orgs/nodejs/discussions/44975