Repository: remy/nodemon
Pull request #1751 (2020-08-07 07:50:48)

fix: runOnChangeOnly=true
Updated

1. test/monitor/run.test.js: to run the test which was previously skipped.
2. lib/monitor/run.js: to handle the start on change only case (-C/--on-change-only) by adding an early exit.
3. lib/monitor/watch.js: to precheck config.options.execOptions before accessing script.

MonarchChakri: @remy Did you get time to look at this PR?
stale[bot]: This issue has been automatically marked as idle and stale because it hasn't had any recent activity. It will be automtically closed if no further activity occurs. If you think this is wrong, or the problem still persists, just pop a reply in the comments and @remy will (try!) to follow up.
Thank you for contributing <3


Patch:
diff --git a/bin/postinstall.js b/bin/postinstall.js
index 70d2a25..c5c35ea 100755
--- a/bin/postinstall.js
+++ b/bin/postinstall.js
@@ -1,7 +1,7 @@
 #!/usr/bin/env node
 
 function main() {
-  if (process.env.SUPPRESS_SUPPORT) {
+  if (process.env.SUPPRESS_SUPPORT || process.env.OPENCOLLECTIVE_HIDE) {
     return;
   }
 
diff --git a/lib/config/load.js b/lib/config/load.js
index ddec54f..bd5a03d 100644
--- a/lib/config/load.js
+++ b/lib/config/load.js
@@ -74,7 +74,9 @@ function load(settings, options, config, callback) {
           }
           // if the script is found as a result of not being on the command
           // line, then we move any of the pre double-dash args in execArgs
-          const n = options.scriptPosition || options.args.length;
+          const n = options.scriptPosition === null ?
+            options.args.length : options.scriptPosition;
+
           options.execArgs = (options.execArgs || [])
             .concat(options.args.splice(0, n));
           options.scriptPosition = null;
diff --git a/lib/monitor/run.js b/lib/monitor/run.js
index dce4638..5060378 100644
--- a/lib/monitor/run.js
+++ b/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'];
@@ -97,6 +112,8 @@ function run(options) {
     utils.version.major > 4 // only fork if node version > 4
 
   if (shouldFork) {
+    // this assumes the first argument is the script and slices it out, since
+    // we're forking
     var forkArgs = cmd.args.slice(1);
     var env = utils.merge(options.execOptions.env, process.env);
     stdio.push('ipc');
@@ -235,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) {
@@ -379,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) {
diff --git a/lib/monitor/watch.js b/lib/monitor/watch.js
index be77c29..1ef1408 100644
--- a/lib/monitor/watch.js
+++ b/lib/monitor/watch.js
@@ -177,7 +177,7 @@ function filterAndRestart(files) {
 
     // if there's no matches, then test to see if the changed file is the
     // running script, if so, let's allow a restart
-    if (config.options.execOptions.script) {
+    if (config.options.execOptions && config.options.execOptions.script) {
       const script = path.resolve(config.options.execOptions.script);
       if (matched.result.length === 0 && script) {
         const length = script.length;


Test patch:
diff --git a/test/config/load.test.js b/test/config/load.test.js
index 083572d..623f2ca 100644
--- a/test/config/load.test.js
+++ b/test/config/load.test.js
@@ -298,4 +298,27 @@ describe('config load', function () {
       done();
     })
   });
+
+  it('should support pkg.main and keep user args on args', done => {
+    process.chdir(path.resolve(pwd, 'test/fixtures/packages/main-and-start'));
+    const settings = { scriptPosition: 0, script: null, args: [ 'first', 'second' ] };
+    const options = { ignore: [], watch: [], monitor: [] };
+    const config = {
+      run: false,
+      system: { cwd: '/Users/remy/dev/nodemon/issues/1758' },
+      required: false,
+      dirs: [],
+      timeout: 1000,
+      options: { ignore: [], watch: [], monitor: [] },
+      lastStarted: 0,
+      loaded: []
+    }
+
+    load(settings, options, config, res => {
+      assert.deepEqual(res.execOptions.args, ['first', 'second']);
+      done();
+    })
+  });
+
+
 });
diff --git a/test/fixtures/packages/main-and-start/index.js b/test/fixtures/packages/main-and-start/index.js
new file mode 100644
index 0000000..e69de29
diff --git a/test/fixtures/packages/main-and-start/package.json b/test/fixtures/packages/main-and-start/package.json
new file mode 100644
index 0000000..f249d17
--- /dev/null
+++ b/test/fixtures/packages/main-and-start/package.json
@@ -0,0 +1,6 @@
+{
+  "main": "./index.js",
+  "scripts": {
+    "start": "node index.js"
+  }
+}
diff --git a/test/monitor/run.test.js b/test/monitor/run.test.js
index b7cc352..3c55bb5 100644
--- a/test/monitor/run.test.js
+++ b/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();
       });