From 7e00a30d31c5a464f7a2bee0657a4aa5e702236a Mon Sep 17 00:00:00 2001 From: Chakradhar Palaparthi Date: Mon, 5 Oct 2020 01:07:50 +0530 Subject: [PATCH] fix: runOnChangeOnly=true Fxies issue #1742: corrected run.js, run.test.js, watch.js for the use case runOnChangeOnly=true (#1751) --- lib/monitor/run.js | 121 ++++++++++++++++++++++----------------- test/monitor/run.test.js | 14 +++-- 2 files changed, 77 insertions(+), 58 deletions(-) diff --git a/lib/monitor/run.js b/lib/monitor/run.js index 8dc10b31..50603787 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']; @@ -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) { diff --git a/test/monitor/run.test.js b/test/monitor/run.test.js index b7cc3521..3c55bb55 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(); });