fix: runOnChangeOnly=true

Fxies issue #1742: corrected run.js, run.test.js, watch.js for the use case runOnChangeOnly=true (#1751)

Author: MonarchChakri

lib/monitor/run.js

@@ -18,16 +18,31 @@ var signals = require('./signals');
 
 function run(options) {
   var cmd = config.command.raw;
+  // moved up
+  // we need restart function below in the global scope for run.kill
+  /*jshint validthis:true*/
+  restart = run.bind(this, options);
+  run.restart = restart;
+
+  // binding options with instance of run
+  // so that we can use it in run.kill
+  run.options = options;
 
   var runCmd = !options.runOnChangeOnly || config.lastStarted !== 0;
   if (runCmd) {
     utils.log.status('starting `' + config.command.string + '`');
+  } else {
+    // should just watch file if command is not to be run
+    // had another alternate approach
+    // to stop process being forked/spawned in the below code
+    // but this approach does early exit and makes code cleaner
+    debug('start watch on: %s', config.options.watch);
+    if (config.options.watch !== false) {
+      watch();
+      return;
+    }
   }
 
-  /*jshint validthis:true*/
-  restart = run.bind(this, options);
-  run.restart = restart;
-
   config.lastStarted = Date.now();
 
   var stdio = ['pipe', 'pipe', 'pipe'];
@@ -237,53 +252,9 @@ function run(options) {
     }
   });
 
-  run.kill = function (noRestart, callback) {
-    // I hate code like this :(  - Remy (author of said code)
-    if (typeof noRestart === 'function') {
-      callback = noRestart;
-      noRestart = false;
-    }
-
-    if (!callback) {
-      callback = noop;
-    }
-
-    if (child !== null) {
-      // if the stdin piping is on, we need to unpipe, but also close stdin on
-      // the child, otherwise linux can throw EPIPE or ECONNRESET errors.
-      if (options.stdin) {
-        process.stdin.unpipe(child.stdin);
-      }
-
-      // For the on('exit', ...) handler above the following looks like a
-      // crash, so we set the killedAfterChange flag if a restart is planned
-      if (!noRestart) {
-        killedAfterChange = true;
-      }
-
-      /* Now kill the entire subtree of processes belonging to nodemon */
-      var oldPid = child.pid;
-      if (child) {
-        kill(child, config.signal, function () {
-          // this seems to fix the 0.11.x issue with the "rs" restart command,
-          // though I'm unsure why. it seems like more data is streamed in to
-          // stdin after we close.
-          if (child && options.stdin && child.stdin && oldPid === child.pid) {
-            child.stdin.end();
-          }
-          callback();
-        });
-      }
-    } else if (!noRestart) {
-      // if there's no child, then we need to manually start the process
-      // this is because as there was no child, the child.on('exit') event
-      // handler doesn't exist which would normally trigger the restart.
-      bus.once('start', callback);
-      restart();
-    } else {
-      callback();
-    }
-  };
+  // moved the run.kill outside to handle both the cases
+  // intial start
+  // no start
 
   // connect stdin to the child process (options.stdin is on by default)
   if (options.stdin) {
@@ -381,12 +352,54 @@ function kill(child, signal, callback) {
   }
 }
 
-// stubbed out for now, filled in during run
-run.kill = function (flag, callback) {
-  if (callback) {
+run.kill = function (noRestart, callback) {
+  // I hate code like this :(  - Remy (author of said code)
+  if (typeof noRestart === 'function') {
+    callback = noRestart;
+    noRestart = false;
+  }
+
+  if (!callback) {
+    callback = noop;
+  }
+
+  if (child !== null) {
+    // if the stdin piping is on, we need to unpipe, but also close stdin on
+    // the child, otherwise linux can throw EPIPE or ECONNRESET errors.
+    if (run.options.stdin) {
+      process.stdin.unpipe(child.stdin);
+    }
+
+    // For the on('exit', ...) handler above the following looks like a
+    // crash, so we set the killedAfterChange flag if a restart is planned
+    if (!noRestart) {
+      killedAfterChange = true;
+    }
+
+    /* Now kill the entire subtree of processes belonging to nodemon */
+    var oldPid = child.pid;
+    if (child) {
+      kill(child, config.signal, function () {
+        // this seems to fix the 0.11.x issue with the "rs" restart command,
+        // though I'm unsure why. it seems like more data is streamed in to
+        // stdin after we close.
+        if (child && run.options.stdin && child.stdin && oldPid === child.pid) {
+          child.stdin.end();
+        }
+        callback();
+      });
+    }
+  } else if (!noRestart) {
+    // if there's no child, then we need to manually start the process
+    // this is because as there was no child, the child.on('exit') event
+    // handler doesn't exist which would normally trigger the restart.
+    bus.once('start', callback);
+    run.restart();
+  } else {
     callback();
   }
 };
+
 run.restart = noop;
 
 bus.on('quit', function onQuit(code) {

test/monitor/run.test.js

@@ -140,17 +140,23 @@ describe('when nodemon runs (2)', function () {
     });
   });
 
-  // FIXME this test was never working properly
-  it.skip('should not run command on startup if runOnChangeOnly is true',
+  // Fixed! FIXME this test was previously not working properly
+  // corrected the test case
+  // script should not be run i.e,
+  // file should not be created 
+  it('should not run command on startup if runOnChangeOnly is true',
     function (done) {
-      fs.writeFileSync(tmp, 'console.log("testing 1 2 3")');
+      var script =  "var touch = require('touch');\n"
+                    + "touch.sync(" + tmp2 + ");\n"
+      fs.writeFileSync(tmp, script);
 
       nodemon({
         script: tmp,
         runOnChangeOnly: true,
         stdout: false,
       }).on('start', function () {
-        assert(false, 'script should not start');
+        // file exists check
+      	assert(!fs.existsSync(tmp2), 'script should not start');
       }).once('exit', function () {
         done();
       });