From c4c36116cfadfa71dedb334f5e263f8163be5290 Mon Sep 17 00:00:00 2001 From: Peter Cottle Date: Mon, 14 Sep 2015 09:57:43 -0700 Subject: [PATCH] Fix various issues with packager editor launcher Summary: There are a few small bugs with the code that launches the editor from the packager: * First of all, the filepath is not escaped which means tokens like `(` or spaces will mess up the process execution. Dropbox unfortunately decided to use spaces in its enterprise product, so I was getting this error: ![screen shot 2015-07-11 at 3 20 54 pm](https://cloud.githubusercontent.com/assets/1135007/8635748/186e7f2e-27ea-11e5-8058-1f4dabb79634.png) * Next, the line number argument formatting was assumed to be in a specific format (`:%d`) which actually errors out vim and other editors. * Lastly, the process was started synchronously but not attached to the stdin / stdout of the parent process. This means that only editors like mvim, sublime, and others would work since they spawn a new window. Editors like emacs, vi, nano, etc wouldn't work and instead just hang at the command line. So I whipped up this diff to fix a number of these issues, demo here: http://recordit.co/M6zwiUj7hp The demo shows both Closes https://github.com/facebook/react-native/pull/1957 Reviewed By: @vjeux, @pcottle Differential Revision: D2420941 Pulled By: @frantic --- launchEditor.js | 77 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 62 insertions(+), 15 deletions(-) diff --git a/launchEditor.js b/launchEditor.js index b572b5cb..f98faaf8 100644 --- a/launchEditor.js +++ b/launchEditor.js @@ -10,7 +10,37 @@ var chalk = require('chalk'); var fs = require('fs'); -var exec = require('child_process').exec; +var spawn = require('child_process').spawn; + +function isTerminalEditor(editor) { + switch (editor) { + case 'vim': + case 'emacs': + case 'nano': + return true; + } + return false; +} + +function getArgumentsForLineNumber(editor, fileName, lineNumber) { + switch (editor) { + case 'vim': + case 'mvim': + return [fileName, '+' + lineNumber]; + case 'atom': + case 'subl': + case 'sublime': + return [fileName + ':' + lineNumber]; + case 'joe': + case 'emacs': + return ['+' + lineNumber, fileName]; + } + + // For all others, drop the lineNumber until we have + // a mapping above, since providing the lineNumber incorrectly + // can result in errors or confusing behavior. + return [fileName]; +} function printInstructions(title) { console.log([ @@ -25,28 +55,45 @@ function printInstructions(title) { ].join('\n')); } +var _childProcess = null; function launchEditor(fileName, lineNumber) { if (!fs.existsSync(fileName)) { return; } - var argument = fileName; - if (lineNumber) { - argument += ':' + lineNumber; + var editor = process.env.REACT_EDITOR || process.env.EDITOR; + if (!editor) { + printInstructions('PRO TIP'); + return; } - var editor = process.env.REACT_EDITOR || process.env.EDITOR; - if (editor) { - console.log('Opening ' + chalk.underline(fileName) + ' with ' + chalk.bold(editor)); - exec(editor + ' ' + argument, function(error) { - if (error) { - console.log(chalk.red(error.message)); - printInstructions('How to fix'); - } - }); - } else { - printInstructions('PRO TIP'); + var args = [fileName]; + if (lineNumber) { + args = getArgumentsForLineNumber(editor, fileName, lineNumber); } + console.log('Opening ' + chalk.underline(fileName) + ' with ' + chalk.bold(editor)); + + if (_childProcess && isTerminalEditor(editor)) { + // There's an existing editor process already and it's attached + // to the terminal, so go kill it. Otherwise two separate editor + // instances attach to the stdin/stdout which gets confusing. + _childProcess.kill('SIGKILL'); + } + + _childProcess = spawn(editor, args, {stdio: 'inherit'}); + _childProcess.on('exit', function(errorCode) { + _childProcess = null; + + if (errorCode) { + console.log(chalk.red('Your editor exited with an error!')); + printInstructions('Keep these instructions in mind:'); + } + }); + + _childProcess.on('error', function(error) { + console.log(chalk.red(error.message)); + printInstructions('How to fix:'); + }) } module.exports = launchEditor;