Skip to content

watch: fix watch args not being properly filtered #57936

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

Merged
merged 1 commit into from
May 5, 2025
Merged

watch: fix watch args not being properly filtered #57936

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions lib/internal/main/watch_mode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,26 @@ const argsWithoutWatchOptions = [];

for (let i = 0; i < process.execArgv.length; i++) {
const arg = process.execArgv[i];
if (StringPrototypeStartsWith(arg, '--watch')) {
i++;
const nextArg = process.execArgv[i];
if (nextArg && nextArg[0] === '-') {
ArrayPrototypePush(argsWithoutWatchOptions, nextArg);
if (StringPrototypeStartsWith(arg, '--watch=')) {
continue;
}
if (arg === '--watch') {
const nextArg = process.execArgv[i + 1];
if (nextArg && nextArg[0] !== '-') {
// If `--watch` doesn't include `=` and the next
// argument is not a flag then it is interpreted as
// the watch argument, so we need to skip that as well
i++;
}
continue;
}
if (StringPrototypeStartsWith(arg, '--watch-path')) {
const lengthOfWatchPathStr = 12;
if (arg[lengthOfWatchPathStr] !== '=') {
// if --watch-path doesn't include `=` it means
// that the next arg is the target path, so we
// need to skip that as well
i++;
}
continue;
}
Expand Down
50 changes: 50 additions & 0 deletions test/sequential/test-watch-mode.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,4 +791,54 @@ process.on('message', (message) => {
`Completed running ${inspect(file)}`,
]);
});

it('when multiple `--watch` flags are provided should run as if only one was', 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.

Note: maybe node should error if multiple --watch flags are provided?

I did not go with such a solution because that would be a breaking change, just ignoring extra --watch flags is a non breaking change and it solves the issue at hand

If someone believes that multiple --watch flags should cause an error I would propose to first land this change right now as a patch then as a followup I can also add the stricter validation which will be included as a breaking change in the next major 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

And/or maybe, the erroring when multiple --watch flags are provided is part of the bigger issues of node being quite permissive with its cli flags: #57864 and could be addressed as part of that 🤔

Copy link
Member

@joyeecheung joyeecheung May 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure if it's this PR, but test-watch-mode has been extremely flaky in both Jenkins and GitHub actions recently e.g. https://ci.nodejs.org/job/node-test-commit/79645/ https://ci.nodejs.org/job/node-test-commit/79644/ and many others. Please refrain from appending new test cases in existing test files as https://github.com/nodejs/node/blob/main/doc/contributing/writing-tests.md suggests - especially this file because it's already being ignored by Jenkins so you are not going to get any proper coverage by appending test cases here, it's only going to make the CI more orange than before, and when it increases flakiness you won't be able to notice.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, thank you very much and sorry for the trouble, I will keep this in mind and create tests in new files whenever possible 🙇

const projectDir = tmpdir.resolve('project-multi-flag');
mkdirSync(projectDir);

const file = createTmpFile(`
console.log(
process.argv.some(arg => arg === '--watch')
? 'Error: unexpected --watch args present'
: 'no --watch args present'
);`, '.js', projectDir);
const args = ['--watch', '--watch', file];
const { stdout, stderr } = await runWriteSucceed({
file, watchedFile: file, watchFlag: null, args, options: { cwd: projectDir }
});

assert.strictEqual(stderr, '');
assert.deepStrictEqual(stdout, [
'no --watch args present',
`Completed running ${inspect(file)}`,
`Restarting ${inspect(file)}`,
'no --watch args present',
`Completed running ${inspect(file)}`,
]);
});

it('`--watch-path` ars without `=` used alongside `--watch` should not make it into the script', async () => {
const projectDir = tmpdir.resolve('project-watch-watch-path-args');
mkdirSync(projectDir);

const file = createTmpFile(`
console.log(
process.argv.slice(2).some(arg => arg.endsWith('.js'))
? 'some cli args end with .js'
: 'no cli arg ends with .js'
);`, '.js', projectDir);
const args = ['--watch', `--watch-path`, file, file];
const { stdout, stderr } = await runWriteSucceed({
file, watchedFile: file, watchFlag: null, args, options: { cwd: projectDir }
});

assert.strictEqual(stderr, '');
assert.deepStrictEqual(stdout, [
'no cli arg ends with .js',
`Completed running ${inspect(file)}`,
`Restarting ${inspect(file)}`,
'no cli arg ends with .js',
`Completed running ${inspect(file)}`,
]);
});
});
Loading