watch: track worker thread entry files in --watch mode

[SudhansuBandha]

Currently, --watch mode only tracks dependencies from the main module graph (require/import). Worker thread entry points created via new Worker() are not included, so changes to worker files do not trigger restarts.

This change hooks into Worker initialization and registers the worker entry file with the watch mode, ensuring restarts when worker files change.

Fixes: #62275

[clemyan]

This solves the issue for the worker module itself, but not its dependencies. I.e. if a worker module imports/requires another module, modifying the latter still does not trigger re-run.

[SudhansuBandha]

I have updated changes to watch dependencies for worker as well, so changes in imported/required modules will now trigger a re-run.
@clemyan

[clemyan]

This is the wrong way to solve this, for a myriad of reasons: (based on a cursory glance of the code. Did not bother to actual test it)

1. It is impossible to properly parse JS with regex. This approach cannot even distinguish if the "import" is part of a string or a comment
2. It only works for direct dependencies of the worker module, not transitively. (I.e. it does not watch dependencies of dependencies, and their dependencies, etc.)
3. It only works for literals specifiers that are relative paths, not for absolute paths require('/path/to/file.js'), packages require('my-pkg/sub/path'), dynamic specifier require('./messages/'+language+'/string.json'), or paths that are resolved by customization hooks
4. What about the worker module itself? If it is loaded by customization hooks, there isn't necessarily a file on disk to readSync from.

Any solution must be integrated into Node's loading pipeline

[SudhansuBandha]

I will revert back these updates and work on the suggestions
@clemyan

[franciscoarturorivera371-cyber]

Submit the change.

[SudhansuBandha]

@clemyan
I have updated handling of dependencies from worker thread. For this I have also reverted my changes from the initial commit as well f9beab0
I have implemented the solution for both cjs and esm files.
Do let me know your feedback on the proposed solution and whether it makes sense to proceed further in this direction.

[aduh95]

The added test already passes on main, that means either the code change is not necessary, or that it's not actually testing what you're trying to achieve

[SudhansuBandha]

Sure!! @aduh95 I will update the tests for this. Thanks for your feedback!!

[SudhansuBandha]

@aduh95 Please do review the updated tests for the proposed changes based on behaviour of worker file and it's dependencies in --watch mode for both cjs and esm modules.

[aduh95]

Can you not put the tests in test/sequential/test-watch-mode.mjs? That file is enormous, which is becoming a maintenance issue/

[SudhansuBandha]

@aduh95 As suggested I have removed tests from test/sequential/test-watch-mode.mjs to a new file and made few lint fixes.
Do let me know if any further changes are required

[codecov]

Codecov Report

❌ Patch coverage is 93.61702% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.62%. Comparing base (acb1bd7) to head (a48176d).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/modules/cjs/loader.js	87.50%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #62368      +/-   ##
==========================================
- Coverage   91.43%   89.62%   -1.82%     
==========================================
  Files         356      706     +350     
  Lines      150580   219244   +68664     
  Branches    23602    42007   +18405     
==========================================
+ Hits       137686   196496   +58810     
- Misses      12625    14660    +2035     
- Partials      269     8088    +7819     

Files with missing lines	Coverage Δ
lib/internal/modules/esm/loader.js	99.69% <100.00%> (+38.37%)	⬆️
lib/internal/worker.js	96.68% <100.00%> (+0.20%)	⬆️
lib/internal/modules/cjs/loader.js	98.25% <87.50%> (+18.64%)	⬆️

... and 471 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.

[aduh95]

/cc @nodejs/fs

[SudhansuBandha]

Thank you @aduh95 for your support so far in making necessary updates to the PR and running the CI pipeline.
I have made necessary updates as per latest feedback. Do let me know if any further changes are needed.

[SudhansuBandha]

Can we add label fs to the PR?
@nodejs-github-bot

[clemyan]

I can confirm this solves my issue, but this does cause additional userland messages to be sent on the parentPort of workers run under watch mode. I'll leave deciding whether this is an issue to the Node team.

[SudhansuBandha]

Hi @avivkeller @lpinca and @geeksilva97 When you have a moment, could you please take a look at this PR and share your feedback? Thank you so much!

[SudhansuBandha]

@MoLow Thank you for your approval. I have rebased my branch with main. If possible please do re run the CI pipeline.

[SudhansuBandha]

@MoLow Thank you for the re run. Since it is passing all the checks, is it good to be merged?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/72882/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/72886/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/72888/

[MoLow]

@SudhansuBandha ci failures might be related and allwys fail the same tests, can you kindly investigate?