Skip to content

Commit

Permalink
Fix various issues with packager editor launcher
Browse files Browse the repository at this point in the history
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 #1957

Reviewed By: @vjeux, @pcottle

Differential Revision: D2420941

Pulled By: @frantic
  • Loading branch information
pcottle authored and facebook-github-bot-7 committed Sep 14, 2015
1 parent 515d5a5 commit c372dab
Showing 1 changed file with 61 additions and 14 deletions.
75 changes: 61 additions & 14 deletions packager/launchEditor.js
Original file line number Diff line number Diff line change
Expand Up @@ -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([
Expand All @@ -25,28 +55,45 @@ function printInstructions(title) {
].join('\n'));
}

var _childProcess = null;
function launchEditor(fileName, lineNumber) {
if (!fs.existsSync(fileName)) {
return;
}

var argument = fileName;
var editor = process.env.REACT_EDITOR || process.env.EDITOR;
if (!editor) {
printInstructions('PRO TIP');
return;
}

var args = [fileName];
if (lineNumber) {
argument += ':' + lineNumber;
args = getArgumentsForLineNumber(editor, fileName, lineNumber);
}
console.log('Opening ' + chalk.underline(fileName) + ' with ' + chalk.bold(editor));

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');
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;

0 comments on commit c372dab

Please sign in to comment.