Directly restart Rollup when config file change is detected

[lukastaegert]

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:

Description

As there was another test failure on a branch, this one picks up a suggestion by @kzc to use fs.fsync to make sure an updated file is flushed to disk earlier.

[github-actions]

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#improve-watch-test

or load it into the REPL:
https://rollupjs.org/repl/?pr=4344

[codecov]

Codecov Report

Merging #4344 (8f85dfa) into master (9e947fc) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4344      +/-   ##
==========================================
- Coverage   98.43%   98.43%   -0.01%     
==========================================
  Files         206      206              
  Lines        7350     7344       -6     
  Branches     2088     2088              
==========================================
- Hits         7235     7229       -6     
  Misses         55       55              
  Partials       60       60              

Impacted Files	Coverage Δ
cli/logging.ts	83.33% <100.00%> (ø)
cli/run/watch-cli.ts	89.70% <100.00%> (-0.84%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e947fc...8f85dfa. Read the comment docs.

[kzc]

In spite of the fs.fsyncSync, I still see one failure on macos/node12 with a repeat of 10. Between node12 and node16 on mac we have 1 failure in 20 combined test runs - a 5% failure rate for that test. Just for the heck of it, could you change the repeat to 40 and initial timeout from 3000 to 4000 to see what happens?

[kzc]

Hmm... it passed. I wonder if the fsync made any difference.

[kzc]

In my opinion fs.watch shouldn't be used in test validation since the following error is not a rollup failure, but a failure of this construction of the test or test harness itself:

  14) rollup
       cli
         watch
           watch-config-early-update: immediately reloads the config file if a change happens while it is parsed (pass 27):
     Error: ENOENT: no such file or directory, lstat '/Users/runner/work/rollup/rollup/test/cli/samples/watch/watch-config-early-update/_actual'

The original random failures for this particular test was that it was producing _actual/output1.js instead of the expected _actual/output2.js result, but the _actual directory not existing was not an issue.

Instead of a Promise.race between a timeout and fs.watch, just the timeout is sufficient and is simpler to reason about - particularly since we suspect that fs.watch used by rollup on Mac is faulty.

[lukastaegert]

In my opinion fs.watch shouldn't be used in test validation since the following error is not a rollup failure, but a failure of this construction of the test or test harness itself:

No, it means that no output was generated as _actual is created the first time Rollup generates output. But it look misleading to be sure.

When you get _actual/output1.js, then it means the config resolved too early, i.e. before Rollup's own watcher detected the config file updated. Otherwise, it would not have bundled the first config.

The race was more like an experiment. As it turned out, fs.watch does not trigger at all quite often, and that very much looks like a bug. Using chokidar in the config helps, but breaks 100% on Windows for reasons I can only guess.

[kzc]

When you get _actual/output1.js, then it means the config resolved too early, i.e. before Rollup's own watcher detected the config file updated. Otherwise, it would not have bundled the first config.

Yes, that's consistent with my understanding as well. :-)

No, it means that no output was generated as _actual is created the first time Rollup generates output. But it look misleading to be sure.

My point was that the following error on macOS in the second last commit (and whose logic remains in some form in the latest PR commit) had never occurred before in the original randomly failing test on master which always had previously either emitted the erroneous _actual/output1.js output or the correct _actual/output2.js output.

 Error: ENOENT: no such file or directory, lstat '/Users/runner/work/rollup/rollup/test/cli/samples/watch/watch-config-early-update/_actual'

It shows the test's own newly introduced watch event fired incorrectly on macOS. This is why my vote would be to drop the watcher experiment in the test case.

I suspect that watchers on macOS are not 100% reliable in general, and the original test failures on master were demonstrating a genuine watch bug on macOS.

[lukastaegert]

I changed the logic in Rollup to directly rebuild once a config file change is detected instead of waiting for the previous config file to be fully parsed. I think this makes more sense anyway as it allows to cancel a "stuck" config file by changing it. But it also allowed me to change the test so that the config file hooks into process.stderr to detect once Rollup has finished parsing the second config file.
This makes the tests much faster. Unfortunately, they still fail occasionally, which now clearly indicates that in those cases, the chokidar watcher in Rollup did not trigger at all.