Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Speed up editor detection on Windows #2711

Closed
wants to merge 6 commits into from
Closed

Speed up editor detection on Windows #2711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cd28840
Speed up editor detection on Windows
levrik Jul 2, 2017
39289bb
Launch PowerShell agent in on middleware-creation
levrik Jul 30, 2017
55dca23
Only launch PowerShell agent if no editor is configured
levrik Jul 30, 2017
5785eaf
Fixed issue with Promise-based editor detection and cleaned up code
levrik Jul 30, 2017
5b2f6ae
Merge remote-tracking branch 'upstream/master' into speed-up-editor-d…
levrik Sep 7, 2017
1a7907f
Fixed mapping of PS output to right Promise
levrik Sep 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/react-dev-utils/errorOverlayMiddleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,14 @@
*/
'use strict';

const launchEditor = require('./launchEditor');
const { launchEditor, tryLaunchPowerShellAgent } = require('./launchEditor');
const launchEditorEndpoint = require('./launchEditorEndpoint');

module.exports = function createLaunchEditorMiddleware() {
if (process.platform === 'win32' && !process.env.REACT_EDITOR) {
tryLaunchPowerShellAgent();
}

return function launchEditorMiddleware(req, res, next) {
if (req.url.startsWith(launchEditorEndpoint)) {
launchEditor(req.query.fileName, req.query.lineNumber);
Expand Down
265 changes: 171 additions & 94 deletions packages/react-dev-utils/launchEditor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,62 @@
const fs = require('fs');
const path = require('path');
const child_process = require('child_process');
const EventEmitter = require('events').EventEmitter;
const os = require('os');
const chalk = require('chalk');
const shellQuote = require('shell-quote');

// Inspired by https://github.com/rannn505/node-powershell
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason we're not just using it?

Copy link
Contributor Author

@levrik levrik Sep 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that module you have an method addCommand and an method invoke.
With addCommand you can add one to many commands and execute them with invoke at once, getting the output of all commands. I wanted a clear connection between a command and its result.

Copy link
Contributor

@Timer Timer Sep 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could send a pull request to the module, adding an execute shortcut which calls addCommand and invoke?

const EOI = 'EOI';
class PowerShell extends EventEmitter {
constructor() {
super();

this._proc = child_process.spawn(
'powershell.exe',
['-NoLogo', '-NoProfile', '-NoExit', '-Command', '-'],
{
stdio: 'pipe',
}
);

this._proc.on('error', () => {
// Needs to be registered... already catched by if-statement below
});

if (!this._proc.pid) {
throw new Error('Failed to start PowerShell');
}

// Initialize counters for mapping events
this._callbackCounter = 0;
this._resolveCounter = 0;

let output = [];
this._proc.stdout.on('data', data => {
if (data.indexOf(EOI) !== -1) {
const eventName = 'resolve' + ++this._callbackCounter;
this.emit(eventName, output.join(''));
output = [];
} else {
output.push(data);
}
});
}

invoke(cmd) {
return new Promise(resolve => {
const eventName = 'resolve' + ++this._resolveCounter;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you try without using EventEmitter? I find code built on events (especially with dynamic names) easy to mess up by accidentally triggering or listening to a wrong event. I would prefer plain callback lists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

this.once(eventName, resolve);

this._proc.stdin.write(cmd);
this._proc.stdin.write(os.EOL);
this._proc.stdin.write(`echo ${EOI}`);
this._proc.stdin.write(os.EOL);
});
}
}

function isTerminalEditor(editor) {
switch (editor) {
case 'vim':
Expand Down Expand Up @@ -43,7 +95,7 @@ const COMMON_EDITORS_OSX = {
'/Applications/CLion.app/Contents/MacOS/clion':
'/Applications/CLion.app/Contents/MacOS/clion',
'/Applications/IntelliJ IDEA.app/Contents/MacOS/idea':
'/Applications/IntelliJ IDEA.app/Contents/MacOS/idea',
'/Applications/IntelliJ IDEA.app/Contents/MacOS/idea',
'/Applications/PhpStorm.app/Contents/MacOS/phpstorm':
'/Applications/PhpStorm.app/Contents/MacOS/phpstorm',
'/Applications/PyCharm.app/Contents/MacOS/pycharm':
Expand All @@ -53,7 +105,7 @@ const COMMON_EDITORS_OSX = {
'/Applications/RubyMine.app/Contents/MacOS/rubymine':
'/Applications/RubyMine.app/Contents/MacOS/rubymine',
'/Applications/WebStorm.app/Contents/MacOS/webstorm':
'/Applications/WebStorm.app/Contents/MacOS/webstorm',
'/Applications/WebStorm.app/Contents/MacOS/webstorm',
};

const COMMON_EDITORS_WIN = [
Expand Down Expand Up @@ -138,57 +190,78 @@ function getArgumentsForLineNumber(editor, fileName, lineNumber, workspace) {
return [fileName];
}

function guessEditor() {
// Explicit config always wins
if (process.env.REACT_EDITOR) {
return shellQuote.parse(process.env.REACT_EDITOR);
let powerShellAgent = null;
function tryLaunchPowerShellAgent() {
if (!powerShellAgent) {
try {
powerShellAgent = new PowerShell();
} catch (err) {
// Failed to start, ignore silent...
powerShellAgent = null;
}
}
}

// Using `ps x` on OSX or `Get-Process` on Windows we can find out which editor is currently running.
// Potentially we could use similar technique for Linux
try {
function guessEditor() {
return Promise.resolve().then(() => {
// Using `ps x` on OSX or `Get-Process` on Windows we can find out which editor is currently running.
// Potentially we could use similar technique for Linux
if (process.platform === 'darwin') {
const output = child_process.execSync('ps x').toString();
const processNames = Object.keys(COMMON_EDITORS_OSX);
for (let i = 0; i < processNames.length; i++) {
const processName = processNames[i];
if (output.indexOf(processName) !== -1) {
return [COMMON_EDITORS_OSX[processName]];
try {
const output = child_process.execSync('ps x').toString();
const processNames = Object.keys(COMMON_EDITORS_OSX);
for (let i = 0; i < processNames.length; i++) {
const processName = processNames[i];
if (output.indexOf(processName) !== -1) {
return COMMON_EDITORS_OSX[processName];
}
}
} catch (error) {
// Ignore...
}
} else if (process.platform === 'win32') {
const output = child_process
.execSync('powershell -Command "Get-Process | Select-Object Path"', {
stdio: ['pipe', 'pipe', 'ignore'],
})
.toString();
const runningProcesses = output.split('\r\n');
for (let i = 0; i < runningProcesses.length; i++) {
// `Get-Process` sometimes returns empty lines
if (!runningProcesses[i]) {
continue;
}
} else if (process.platform === 'win32' && powerShellAgent) {
return powerShellAgent
.invoke('Get-Process | Select-Object Path')
.then(output => {
const runningProcesses = output.split('\r\n');
for (let i = 0; i < runningProcesses.length; i++) {
// `Get-Process` sometimes returns empty lines
if (!runningProcesses[i]) {
continue;
}

const fullProcessPath = runningProcesses[i].trim();
const shortProcessName = path.basename(fullProcessPath);
const fullProcessPath = runningProcesses[i].trim();
const shortProcessName = path.basename(fullProcessPath);

if (COMMON_EDITORS_WIN.indexOf(shortProcessName) !== -1) {
return [fullProcessPath];
}
}
if (COMMON_EDITORS_WIN.indexOf(shortProcessName) !== -1) {
return fullProcessPath;
}
}
});
}
} catch (error) {
// Ignore...
}
});
}

// Last resort, use old skool env vars
if (process.env.VISUAL) {
return [process.env.VISUAL];
} else if (process.env.EDITOR) {
return [process.env.EDITOR];
function tryGetEditor() {
// Explicit config always wins
if (process.env.REACT_EDITOR) {
return Promise.resolve(shellQuote.parse(process.env.REACT_EDITOR));
}

return [null];
return guessEditor().then(editor => {
if (editor) {
return [editor];
} else {
// Last resort, use old skool env vars
if (process.env.VISUAL) {
return [process.env.VISUAL];
} else if (process.env.EDITOR) {
return [process.env.EDITOR];
}

return [null];
}
});
}

function printInstructions(fileName, errorMessage) {
Expand Down Expand Up @@ -229,64 +302,68 @@ function launchEditor(fileName, lineNumber) {
return;
}

let [editor, ...args] = guessEditor();
if (!editor) {
printInstructions(fileName, null);
return;
}

if (
process.platform === 'linux' &&
fileName.startsWith('/mnt/') &&
/Microsoft/i.test(os.release())
) {
// Assume WSL / "Bash on Ubuntu on Windows" is being used, and
// that the file exists on the Windows file system.
// `os.release()` is "4.4.0-43-Microsoft" in the current release
// build of WSL, see: https://github.com/Microsoft/BashOnWindows/issues/423#issuecomment-221627364
// When a Windows editor is specified, interop functionality can
// handle the path translation, but only if a relative path is used.
fileName = path.relative('', fileName);
}
tryGetEditor().then(([editor, ...args]) => {
if (!editor) {
printInstructions(fileName, null);
return;
}

let workspace = null;
if (lineNumber) {
args = args.concat(
getArgumentsForLineNumber(editor, fileName, lineNumber, workspace)
);
} else {
args.push(fileName);
}
if (
process.platform === 'linux' &&
fileName.startsWith('/mnt/') &&
/Microsoft/i.test(os.release())
) {
// Assume WSL / "Bash on Ubuntu on Windows" is being used, and
// that the file exists on the Windows file system.
// `os.release()` is "4.4.0-43-Microsoft" in the current release
// build of WSL, see: https://github.com/Microsoft/BashOnWindows/issues/423#issuecomment-221627364
// When a Windows editor is specified, interop functionality can
// handle the path translation, but only if a relative path is used.
fileName = path.relative('', fileName);
}

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');
}
let workspace = null;
if (lineNumber) {
args = args.concat(
getArgumentsForLineNumber(editor, fileName, lineNumber, workspace)
);
} else {
args.push(fileName);
}

if (process.platform === 'win32') {
// On Windows, launch the editor in a shell because spawn can only
// launch .exe files.
_childProcess = child_process.spawn(
'cmd.exe',
['/C', editor].concat(args),
{ stdio: 'inherit' }
);
} else {
_childProcess = child_process.spawn(editor, args, { stdio: 'inherit' });
}
_childProcess.on('exit', function(errorCode) {
_childProcess = null;
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');
}

if (errorCode) {
printInstructions(fileName, '(code ' + errorCode + ')');
if (process.platform === 'win32') {
// On Windows, launch the editor in a shell because spawn can only
// launch .exe files.
_childProcess = child_process.spawn(
'cmd.exe',
['/C', editor].concat(args),
{ stdio: 'inherit' }
);
} else {
_childProcess = child_process.spawn(editor, args, { stdio: 'inherit' });
}
});
_childProcess.on('exit', function(errorCode) {
_childProcess = null;

_childProcess.on('error', function(error) {
printInstructions(fileName, error.message);
if (errorCode) {
printInstructions(fileName, '(code ' + errorCode + ')');
}
});

_childProcess.on('error', function(error) {
printInstructions(fileName, error.message);
});
});
}

module.exports = launchEditor;
module.exports = {
launchEditor,
tryLaunchPowerShellAgent,
};