Skip to content

watch: watch env files in --watch mode - #54033

Closed
HBSPS wants to merge 3 commits into
nodejs:mainfrom
HBSPS:watch-env-file
Closed

watch: watch env files in --watch mode#54033
HBSPS wants to merge 3 commits into
nodejs:mainfrom
HBSPS:watch-env-file

Conversation

@HBSPS

Copy link
Copy Markdown
Member

observe env files in --watch mode through the watcher's filterFile and forward changes to the childProcess

Fixes: #54001

@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Jul 25, 2024
observe env files in `--watch` mode through the watcher's filterFile and
forward changes to the childProcess
Fixes: nodejs#54001
Comment threadlib/internal/main/watch_mode.js Outdated

let kEnv = {};
for (const arg of process.execArgv) {
if (StringPrototypeStartsWith(arg, '--env-file')) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please use getOptionValue instead of iterating over all of process.execArgv

@HBSPSHBSPSJul 26, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

In fact, I've already applied getOptionValue, but when I used getOptionValue('--env-file') with multiple --env-file options set, I had a problem that I could only get the last --env-file information. Not an array of --env-file information.

Comment on lines +75 to +76
const envContent = parseEnv(readFileSync(envFile, 'utf8'));
kEnv = { ...kEnv, ...envContent };

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
constenvContent=parseEnv(readFileSync(envFile,'utf8'));
kEnv={ ...kEnv, ...envContent};

I am pretty sure that watcher.filterFile is enough since the spawned process should handle --env-file by its own
this is also missing error handling (what happens if env file is renamed/deleted)?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I am pretty sure that watcher.filterFile is enough since the spawned process should handle --env-file by its own

I've tested it many times, but when the child is spwan, it inherits the parent's process.env. If I only use the watcher.filterFile, if the env file is modified, it triggers a restart of the child process, but the information from the modified env doesn't seem to go to the child.

This is why I used parseEnv and readFileSync.

Refs: #54001 (comment)

this is also missing error handling (what happens if env file is renamed/deleted)?

Oh, this is my mistake.

If I can't find the --env-file I set when I started watch mode (because it's renamed or deleted), is it a good way to print ${envFile}: not found like when I first start watch mode?

@anonriganonrig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This PR needs a test

Comment threadlib/internal/main/watch_mode.js
@HBSPS

Copy link
Copy Markdown
MemberAuthor

This PR needs a test

Does this mean I need to write a related test code or does it mean CI workflows in PR?

@anonrig

Copy link
Copy Markdown
Member

This PR needs a test

Does this mean I need to write a related test code or does it mean CI workflows in PR?

You need to write a related test code that covers the feature

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

--env-file watch

5 participants

@HBSPS@anonrig@MoLow@iamrajhans@nodejs-github-bot