Skip to content

util: add loadEnvFile utility #59125

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

util: add loadEnvFile utility #59125

wants to merge 1 commit into from
+201 −95

Conversation

dario-piotrowicz
Copy link
Member

We already have this logic in place for process.loadEnvFile so I figured that it could make sense to just repurpose it also provide a user facing function.

This function is the analogous of util.parseEnv but for files.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/config

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 19, 2025
@dario-piotrowicz dario-piotrowicz force-pushed the dario/util/load-env-file branch from fcc6903 to 08e836c Compare July 19, 2025 17:55
dario-piotrowicz
dario-piotrowicz commented Jul 19, 2025
View reviewed changes
test/parallel/test-process-load-env-file.js
@@ -95,10 +95,10 @@ describe('process.loadEnvFile()', () => {
assert.strictEqual(child.code, 1);
});

it('loadEnvFile does not mutate --env-file output', async () => {
it('loadEnvFile overrides env vars loaded with --env-file', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a breaking change, but I hope it's fine given that dotenv is experimental (and I don't imagine many developers relying on this behavior? 🤔)

I am also not sure about the rational, personally if a new env file is loaded onto process.env I would expect that it would override whatever else is there 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a problem I can keep the existing behavior.

Since I am thinking of making more changes in this space, to avoid churn and I would prefer this PR not to be a semver-major one if possible.

lib/internal/process/per_thread.js
Comment on lines +359 to +360
const loaded = util.loadEnvFile(path);
ObjectAssign(process.env, loaded);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given how trivial it is to load a .env file onto process.env I even wonder if we could deprecate/remove process.loadEnvFile.

To me personally it feels much clearer to have this sort of operation more manual and have the user load their .env files withloadEnvFile and then use the values to populate process.env themselves if and how they want (with any overriding, filtering, etc. logic).

@dario-piotrowicz dario-piotrowicz added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 19, 2025
@dario-piotrowicz dario-piotrowicz force-pushed the dario/util/load-env-file branch from 08e836c to 6d075c6 Compare July 19, 2025 18:06
@codecov Codecov
Copy link

codecov bot commented Jul 19, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 84.61538% with 8 lines in your changes missing coverage. Please review.

Project coverage is 90.05%. Comparing base (c8d5b39) to head (6d075c6).

Files with missing lines Patch % Lines
src/node_util.cc 69.23% 5 Missing and 3 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #59125      +/-   ##
==========================================
+ Coverage   90.03%   90.05%   +0.02%     
==========================================
  Files         648      648              
  Lines      190967   190984      +17     
  Branches    37425    37443      +18     
==========================================
+ Hits       171931   171988      +57     
+ Misses      11665    11625      -40     
  Partials     7371     7371
Files with missing lines Coverage Δ
lib/internal/process/per_thread.js 99.27% <100.00%> (-0.19%) ⬇️
lib/util.js 97.83% <100.00%> (+0.08%) ⬆️
src/node_process.h 25.00% <ø> (ø)
src/node_process_methods.cc 88.55% <ø> (+0.74%) ⬆️
src/node_util.cc 80.74% <69.23%> (-0.93%) ⬇️

... and 40 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dario-piotrowicz @nodejs-github-bot