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.

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

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

Conversation

dario-piotrowicz
Copy link
Member

@dario-piotrowicz dario-piotrowicz commented Apr 19, 2025

Currently the logic for filtering out watch related flags before passing them to the watch target script is flawed, as it includes any flag (starting with -) that follows a flag starting with --watch (code), this allows some watch flags to make it pass the filtering resulting in the generation of multiple watch processes which is both wasteful and can cause duplicate terminal logs (alongside other issues if the target script is side-effectful).

For example if I run

node --watch --watch index.js

the current implementation will spawn a process equivalent to:

node --watch index.js

which will then finally spawn the correct:

node index.js

Here I'm addressing the incorrect filtering so that all the watch flags are correctly filtered out (making the extra process spawning disappear)


Fixes #57124

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 19, 2025
@@ -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

codecov bot commented Apr 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.27%. Comparing base (25842c5) to head (685fb31).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #57936      +/-   ##
==========================================
- Coverage   92.26%   90.27%   -1.99%     
==========================================
  Files         325      630     +305     
  Lines      126673   186112   +59439     
  Branches    20783    36470   +15687     
==========================================
+ Hits       116869   168017   +51148     
- Misses       9576    10976    +1400     
- Partials      228     7119    +6891     

see 412 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.

@dario-piotrowicz dario-piotrowicz changed the title watch: fix --watch args not being properly filtered watch: fix watch args not being properly filtered Apr 19, 2025
@dario-piotrowicz dario-piotrowicz force-pushed the dario/57124/node-watch-args-filtering-fix branch from cdc049a to 4fa1ec0 Compare April 19, 2025 22:35
currently when --watch is used, the argv arguments that
the target script receives are filtered so that they don't
include watch related arguments, however the current
filtering logic is incorrect and it causes some watch values
to incorrectly pass the filtering, the changes here address
such issue
@dario-piotrowicz dario-piotrowicz force-pushed the dario/57124/node-watch-args-filtering-fix branch from 4fa1ec0 to 685fb31 Compare April 19, 2025 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--watch combined with --watch-path results in duplicate restart events/logs
3 participants