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 1 commit
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
249 changes: 155 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,59 @@
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',
}
);

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

this._proc.on('error', () => {
this._proc = null;
});
}

invoke(cmd) {
if (!this._proc) {
return Promise.resolve('');
}

return new Promise(resolve => {
this.on('resolve', data => {
resolve(data);
this.removeAllListeners('resolve');
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any possible race conditions here? What if we call invoke twice before it resolves? Will both be resolved correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhm. I will test that next weekend 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed 🙂

});

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 @@ -104,57 +153,65 @@ 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 launchPowerShellAgent() {
if (!powerShellAgent) {
powerShellAgent = new PowerShell();
}
}

// 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 {
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]];
}
}
} 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;
function guessEditor() {
return new Promise(resolve => {
// Explicit config always wins
if (process.env.REACT_EDITOR) {
return resolve(shellQuote.parse(process.env.REACT_EDITOR));
}

// 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 {
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 resolve([COMMON_EDITORS_OSX[processName]]);
}
}
} 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 resolve([fullProcessPath]);
}
}
});
}
} catch (error) {
// Ignore...
}
} 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];
}
// Last resort, use old skool env vars
if (process.env.VISUAL) {
return resolve([process.env.VISUAL]);
} else if (process.env.EDITOR) {
return resolve([process.env.EDITOR]);
}

return [null];
return resolve([null]);
});
}

function printInstructions(fileName, errorMessage) {
Expand Down Expand Up @@ -195,64 +252,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);
}
guessEditor().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,
launchPowerShellAgent,
};
2 changes: 1 addition & 1 deletion packages/react-error-overlay/middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
*/
'use strict';

const launchEditor = require('react-dev-utils/launchEditor');
const { launchEditor } = require('react-dev-utils/launchEditor');

module.exports = function createLaunchEditorMiddleware() {
return function launchEditorMiddleware(req, res, next) {
Expand Down
6 changes: 6 additions & 0 deletions packages/react-scripts/scripts/start.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,12 @@ choosePort(HOST, DEFAULT_PORT)
}
console.log(chalk.cyan('Starting the development server...\n'));
openBrowser(urls.localUrlForBrowser);
if (process.platform === 'win32') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would still block the first compilation, no? It would be nice to check this (e.g. by artificially putting an infinite loop into the launching code). Perhaps setTimeout would help?

Copy link
Contributor Author

@levrik levrik Jul 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Just tested the time overhead:

console.time('start');
if (process.platform === 'win32') {
  const {
    launchPowerShellAgent,
  } = require('react-dev-utils/launchEditor');
  launchPowerShellAgent();
  console.timeEnd('start');
}

Output was start: 4.349ms

But, yes, it would still "block" the first compilation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I guess the difference is smaller because we're not actually waiting for process output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, right. We're just waiting for the process to gets created.

const {
launchPowerShellAgent,
} = require('react-dev-utils/launchEditor');
launchPowerShellAgent();
}
});

['SIGINT', 'SIGTERM'].forEach(function(sig) {
Expand Down