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

Fail for missing env file in development only #51451

Closed
jmjf opened this issue Jan 13, 2024 · 9 comments
Closed

Fail for missing env file in development only #51451

jmjf opened this issue Jan 13, 2024 · 9 comments
Labels
dotenv Issues and PRs related to .env file parsing feature request Issues that request new features to be added to Node.js.

Comments

@jmjf
Copy link

jmjf commented Jan 13, 2024

What is the problem this feature will solve?

50588 changed behavior of the --env-file option so Node will exit with an error if the env file does not exist. This change was motivated by 50536.

This change breaks npm scripts in CI, where it's normal to have no env file. For example, a test script.

What is the feature you are proposing to solve the problem?

Option 1a: Do not exit with an error unless NODE_ENV=development.

Option 1b: Do not exit with an error if NODE_ENV is not development.

Option 2: Instead of exiting with an error, warn and continue. In 50536, OP suggested an error or a warning, though later said an error would be consistent with node:fs behavior.

Option 3: Revert 50588.

What alternatives have you considered?

Workaround 1: In the CI script, add touch .env or similar to create an empty file. This workaround feels kludgey. (Creating env files in CI. Empty files just to get around a requirement that only consider the development perspective.) It also wouldn't work for Windows CI environments.

Workaround 2: Copy npm scripts that use --env-file and make :ci versions of them for CI without --env-file. This workaround requires duplicate npm scripts that must be kept in sync, which creates an opportunity for divergence between development and CI that can lead to unexpected errors.

Workaround 3: Check the env file into the repo. I hope I don't need to explain why this is a bad idea.

Discussion

50536 and 50588 arguments

50536 gives an example of a monorepo and changing context of the current directory or lacking permissions to read the env file. I may be misreading, but this example sounds like an issue specific to an individual's development environment that would not apply in a CI environment.

50588 asserts that "the env file is a crucial file." This assertion is usually true an individual developer's personal development environment, but false in other environments.

Counterargument

Non-development environments (CI, test/staging, production, etc.) usually get environment variables from the environment, not a file. Applications cannot rely on Node to fail them if an env file is missing in these environments.

Therefore, applications should confirm required environment values are set and have valid values and fail or provide reasonable defaults if they are not. For example, if process.env.REQUIRED_ENV_VARIABLE === undefined { process.exit(1); } or if (Object.hasOwn(process.env, 'REQUIRED_ENV_VARIABLE')) { process.exit(1); }.

Given the monorepo example in 50536, one would suspect the project is non-trivial. The missing permissions point implies the developer can't set ACLs, which sounds like a large organization with robust risk controls. In neither case should the application assume environment variables are set.

Comparing options 1a and 1b

Options 1a and 1b seek to strike a balance between the needs of development and CI by making this behavior dependent on NODE_ENV, but differ in their approach.

Option 1a requires setting NODE_ENV in development to get a failure if the env file doesn't exist.

Option 1b requires setting NODE_ENV in other environments. For example, I'd need to add NODE_ENV=ci (or any other value that isn't development) in my CI environment configuration. It's a little extra work, but doesn't feel kludgey like workaround 1 or add npm scripts like workaround 2.

Option 1b is most consistent with existing patterns. "Node.js assumes it's always running in a development environment." ref. Many packages change logging and other behaviors based on the value of NODE_ENV. (Let's avoid the "to NODE_ENV or not to NODE_ENV" debate, please.)

If 1a and 1b are unacceptable, then options 2 and 3 let us don't need kludgey workarounds or duplicate scripts to make CI work.

@jmjf jmjf added the feature request Issues that request new features to be added to Node.js. label Jan 13, 2024
@anonrig
Copy link
Member

anonrig commented Jan 13, 2024

cc @nodejs/tsc @GeoffreyBooth

@GeoffreyBooth
Copy link
Member

First, we’ve been discouraging use of NODE_ENV for many years now. I don’t see us shipping a solution that relies on that. It’s not one of the supported environment variables.

Second, if you’re running node --env-file=some-file.env and some-file.env isn’t present, Node should error. That’s like running node some-file.js and expecting Node to do something other than error if some-file.js doesn’t exist. I don’t buy the argument that Node should just warn or ignore the error; you asked Node to load a file, and it couldn’t, so it should error.

There’s an assumption implicit in the problem as posed: that the same command needs to be used in both development and CI. Why is that the case? It’s common to run different commands in CI, for example npm ci instead of npm install. I would solve this problem at that level, rather than expecting counterintuitive behavior out of Node.

@benjamingr
Copy link
Member

@GeoffreyBooth

I don’t buy the argument that Node should just warn or ignore the error; you asked Node to load a file, and it couldn’t, so it should error.

I think the reason people are asking for this is that it is the behavior of most libraries that support .env (to ignore a missing .env) since the typical flow is to not check in .env files to source control.

I'm not saying it's good behavior or we should change to it (and I approved the "fail on it" PR) - just explaining why it's the expectation.

@mcollina
Copy link
Member

I'm not ok in adding any support for NODE_ENV in core. It's an anti-pattern.

I'm also ok in always ignoring errors if there is no .env file to load, and print a warning instead.

@anonrig
Copy link
Member

anonrig commented Jan 15, 2024

I'm also ok in always ignoring errors if there is no .env file to load, and print a warning instead.

I'm also ok with a warning

@IlyasShabi
Copy link
Contributor

Is it possible to add some options, specifically to the programmatic API, to quietly handle missing files ? This could be useful for other things down the line too

@GeoffreyBooth
Copy link
Member

Is it possible to add some options, specifically to the programmatic API, to quietly handle missing files ? This could be useful for other things down the line too

The programmatic API has a slightly different purpose; by the time your code is running, it’s impossible to set NODE_OPTIONS (at least, if you want Node to behave according to the options you set). That’s what’s special about --env-file. The main benefit of a programmatic API is to avoid needing to import a library such as dotenv to parse the .env file.

It’s very easy to put a try/catch around a call in JavaScript, so I’m not sure it’s worth the complexity of adding an option for ignoring missing files; though if we do, it should be force: true to disable a default error rather than the reverse, to match the fs APIs. Regardless, this doesn’t solve the original “ignore missing file in CI” request if the file defines NODE_OPTIONS, in which case it has to be loaded via --env-file.

I still think the solution there isn’t for Node to stop erroring on missing files, but rather for the user to use other logic outside of Node to know whether or not --env-file should be passed (or load an empty file in CI). I’m more concerned about the case of an app starting up with required configuration missing than I am about slightly more complexity in CI like touch .env && node --env-file=.env start.js. I just don’t think the need to either ensure that the requested file exists, or alter the command per environment, justifies the potential footgun and the inconsistency with Node’s other APIs.

@anonrig anonrig added the dotenv Issues and PRs related to .env file parsing label Jan 18, 2024
@jaydenseric
Copy link
Contributor

jaydenseric commented Apr 30, 2024

There is an easy non-breaking-change solution: A new Node.js arg --optional-env-file=.env. The arg --env-file would continue with it's current behavior, and --optional-env-file would load the specified env file only if it exists, and not error or log anything if it's missing.

Edit: I now realize there is a feature request issue for exactly that:

#50993

@RedYetiDev
Copy link
Member

This has been superceded by #50993

@RedYetiDev RedYetiDev closed this as not planned Won't fix, can't repro, duplicate, stale Jun 27, 2024
BoscoDomingo added a commit to BoscoDomingo/node that referenced this issue Jun 30, 2024
BoscoDomingo added a commit to BoscoDomingo/node that referenced this issue Aug 25, 2024
Fixes: nodejs#50993
Refs: nodejs#51451

test: remove unnecessary comment

src: conform to style guidelines

src: change flag to `--env-file-optional`

test: revert automatic linter changes

doc: fix typos

src: change flag to `--env-file-if-exists`

src: refactor `env_file_data` and `GetEnvFileDataFromArgs`

test: clean up tests

src: print error when file not found

test: remove unnecessary extras
BoscoDomingo added a commit to BoscoDomingo/node that referenced this issue Aug 31, 2024
Fixes: nodejs#50993
Refs: nodejs#51451

test: remove unnecessary comment

src: conform to style guidelines

src: change flag to `--env-file-optional`

test: revert automatic linter changes

doc: fix typos

src: change flag to `--env-file-if-exists`

src: refactor `env_file_data` and `GetEnvFileDataFromArgs`

test: clean up tests

src: print error when file not found

test: remove unnecessary extras
BoscoDomingo added a commit to BoscoDomingo/node that referenced this issue Aug 31, 2024
Fixes: nodejs#50993
Refs: nodejs#51451

test: remove unnecessary comment

src: conform to style guidelines

src: change flag to `--env-file-optional`

test: revert automatic linter changes

doc: fix typos

src: change flag to `--env-file-if-exists`

src: refactor `env_file_data` and `GetEnvFileDataFromArgs`

test: clean up tests

src: print error when file not found

test: remove unnecessary extras
BoscoDomingo added a commit to BoscoDomingo/node that referenced this issue Aug 31, 2024
Fixes: nodejs#50993
Refs: nodejs#51451

test: remove unnecessary comment

src: conform to style guidelines

src: change flag to `--env-file-optional`

test: revert automatic linter changes

doc: fix typos

src: change flag to `--env-file-if-exists`

src: refactor `env_file_data` and `GetEnvFileDataFromArgs`

test: clean up tests

src: print error when file not found

test: remove unnecessary extras
BoscoDomingo added a commit to BoscoDomingo/node that referenced this issue Sep 1, 2024
Fixes: nodejs#50993
Refs: nodejs#51451

test: remove unnecessary comment

src: conform to style guidelines

src: change flag to `--env-file-optional`

test: revert automatic linter changes

doc: fix typos

src: change flag to `--env-file-if-exists`

src: refactor `env_file_data` and `GetEnvFileDataFromArgs`

test: clean up tests

src: print error when file not found

test: remove unnecessary extras
aduh95 pushed a commit to BoscoDomingo/node that referenced this issue Sep 2, 2024
Fixes: nodejs#50993
Refs: nodejs#51451

test: remove unnecessary comment

src: conform to style guidelines

src: change flag to `--env-file-optional`

test: revert automatic linter changes

doc: fix typos

src: change flag to `--env-file-if-exists`

src: refactor `env_file_data` and `GetEnvFileDataFromArgs`

test: clean up tests

src: print error when file not found

test: remove unnecessary extras
aduh95 pushed a commit to BoscoDomingo/node that referenced this issue Sep 7, 2024
Fixes: nodejs#50993
Refs: nodejs#51451

test: remove unnecessary comment

src: conform to style guidelines

src: change flag to `--env-file-optional`

test: revert automatic linter changes

doc: fix typos

src: change flag to `--env-file-if-exists`

src: refactor `env_file_data` and `GetEnvFileDataFromArgs`

test: clean up tests

src: print error when file not found

test: remove unnecessary extras
nodejs-github-bot pushed a commit that referenced this issue Sep 16, 2024
Fixes: #50993
Refs: #51451

test: remove unnecessary comment

src: conform to style guidelines

src: change flag to `--env-file-optional`

test: revert automatic linter changes

doc: fix typos

src: change flag to `--env-file-if-exists`

src: refactor `env_file_data` and `GetEnvFileDataFromArgs`

test: clean up tests

src: print error when file not found

test: remove unnecessary extras
PR-URL: #53060
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
RafaelGSS pushed a commit that referenced this issue Sep 16, 2024
Fixes: #50993
Refs: #51451

test: remove unnecessary comment

src: conform to style guidelines

src: change flag to `--env-file-optional`

test: revert automatic linter changes

doc: fix typos

src: change flag to `--env-file-if-exists`

src: refactor `env_file_data` and `GetEnvFileDataFromArgs`

test: clean up tests

src: print error when file not found

test: remove unnecessary extras
PR-URL: #53060
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
RafaelGSS pushed a commit that referenced this issue Sep 16, 2024
Fixes: #50993
Refs: #51451

test: remove unnecessary comment

src: conform to style guidelines

src: change flag to `--env-file-optional`

test: revert automatic linter changes

doc: fix typos

src: change flag to `--env-file-if-exists`

src: refactor `env_file_data` and `GetEnvFileDataFromArgs`

test: clean up tests

src: print error when file not found

test: remove unnecessary extras
PR-URL: #53060
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
RafaelGSS pushed a commit that referenced this issue Sep 17, 2024
Fixes: #50993
Refs: #51451

test: remove unnecessary comment

src: conform to style guidelines

src: change flag to `--env-file-optional`

test: revert automatic linter changes

doc: fix typos

src: change flag to `--env-file-if-exists`

src: refactor `env_file_data` and `GetEnvFileDataFromArgs`

test: clean up tests

src: print error when file not found

test: remove unnecessary extras
PR-URL: #53060
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
RafaelGSS pushed a commit that referenced this issue Sep 17, 2024
Fixes: #50993
Refs: #51451

test: remove unnecessary comment

src: conform to style guidelines

src: change flag to `--env-file-optional`

test: revert automatic linter changes

doc: fix typos

src: change flag to `--env-file-if-exists`

src: refactor `env_file_data` and `GetEnvFileDataFromArgs`

test: clean up tests

src: print error when file not found

test: remove unnecessary extras
PR-URL: #53060
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
Fixes: nodejs#50993
Refs: nodejs#51451

test: remove unnecessary comment

src: conform to style guidelines

src: change flag to `--env-file-optional`

test: revert automatic linter changes

doc: fix typos

src: change flag to `--env-file-if-exists`

src: refactor `env_file_data` and `GetEnvFileDataFromArgs`

test: clean up tests

src: print error when file not found

test: remove unnecessary extras
PR-URL: nodejs#53060
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dotenv Issues and PRs related to .env file parsing feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants