Skip to content

Commit

Permalink
fix: runOnChangeOnly=true
Browse files Browse the repository at this point in the history
Fxies issue #1742: corrected run.js, run.test.js, watch.js for the use case runOnChangeOnly=true (#1751)
  • Loading branch information
MonarchChakri authored Oct 4, 2020
1 parent 2967726 commit 7e00a30
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 58 deletions.
121 changes: 67 additions & 54 deletions lib/monitor/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'];
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
14 changes: 10 additions & 4 deletions test/monitor/run.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Expand Down

0 comments on commit 7e00a30

Please sign in to comment.