node-dev icon indicating copy to clipboard operation
node-dev copied to clipboard

Handle multiple file changes.

Open wclr opened this issue 2 years ago • 7 comments

Without it if multiple changes occured at once, it will try to start process multiple times.

wclr avatar Jun 07 '22 10:06 wclr

@bjornstar are you going to review this somehow?

wclr avatar Jan 11 '23 11:01 wclr

@bjornstar see please)

reslear avatar Jan 11 '23 12:01 reslear

I see test failures, and no tests to validate the behavior.

bjornstar avatar Jan 11 '23 12:01 bjornstar

Well, I've tried to reproduce the problem with tests, and could not: watcher.removeAll() in the handler removes watching after just the first change. So the next change is not caught and handled.

I'm pretty sure that in my case I see that multiple file changes happened at once resulted in multiple Restarting for each file. My case is dev app that is running in docker (which requires polling) and it is watching for quite a lot of files. And simultaneous change happen due to re-compilation and emitting a great amount of new files. So, I guess, this could probably occur because watcher removal is not completed to prevent sequential change events from firing and handling. And setting isPaused right after the first change, and then checking it for subsequent change solved this problem (I'm currently using a modified version to avoid this).

Btw if to add:

watcher.on('change', file => {
   if (isPaused) return;
   isPaused = true;

I don't see any current tests failing.

wclr avatar Jan 12 '23 18:01 wclr

@bjornstar So it seems that this case is is not easy to reproduce in tests. But logically it should be understood that this:

watcher.on('change', file => {
   if (isPaused) return;
   isPaused = true;

allows to prevent consequent changes that should not be handled.

What do you think?

wclr avatar Jan 16 '23 11:01 wclr

hi @bjornstar, i use this PR as a patch and test:

  • save many files at the same time
  • switch between branches with many file changes

and works fine 🔥 prevents a lot of unnecessary restarts.

@wclr good work

reslear avatar Apr 28 '23 13:04 reslear

@bjornstar Look at this again please, this is a simple logical change that we will NOT handle any changes as soon as isPaused set, currently it is possible that multiple file changes will pass without such filter and try to restart.

Current:

  watcher.on('change', file => {
    clearOutput();
    notify('Restarting', `${file} has been modified`);
    watcher.removeAll();
    isPaused = true;
    if (child) {
      // Child is still running, restart upon exit
      child.on('exit', start);

With change:

watcher.on('change', file => {
    if (isPaused) return;
    isPaused = true;
    clearOutput();
    notify('Restarting', `${file} has been modified`);
    watcher.removeAll();    
    if (child) {
      // Child is still running, restart upon exit
      child.on('exit', start);

wclr avatar Dec 26 '23 13:12 wclr