From cd288403cb2732e3f2f8b776a54dd174d7fa997a Mon Sep 17 00:00:00 2001 From: Levin Rickert Date: Sun, 2 Jul 2017 17:43:34 +0200 Subject: [PATCH 1/5] Speed up editor detection on Windows before ~600ms after ~300ms --- packages/react-dev-utils/launchEditor.js | 249 +++++++++++++-------- packages/react-error-overlay/middleware.js | 2 +- packages/react-scripts/scripts/start.js | 6 + 3 files changed, 162 insertions(+), 95 deletions(-) diff --git a/packages/react-dev-utils/launchEditor.js b/packages/react-dev-utils/launchEditor.js index bcf0a506069..76cc3e72bb5 100644 --- a/packages/react-dev-utils/launchEditor.js +++ b/packages/react-dev-utils/launchEditor.js @@ -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 +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'); + }); + + 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': @@ -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) { @@ -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, +}; diff --git a/packages/react-error-overlay/middleware.js b/packages/react-error-overlay/middleware.js index d4fd0d399f1..2857eea6023 100644 --- a/packages/react-error-overlay/middleware.js +++ b/packages/react-error-overlay/middleware.js @@ -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) { diff --git a/packages/react-scripts/scripts/start.js b/packages/react-scripts/scripts/start.js index b86943b4d9d..8408a848117 100644 --- a/packages/react-scripts/scripts/start.js +++ b/packages/react-scripts/scripts/start.js @@ -85,6 +85,12 @@ choosePort(HOST, DEFAULT_PORT) } console.log(chalk.cyan('Starting the development server...\n')); openBrowser(urls.localUrlForBrowser); + if (process.platform === 'win32') { + const { + launchPowerShellAgent, + } = require('react-dev-utils/launchEditor'); + launchPowerShellAgent(); + } }); ['SIGINT', 'SIGTERM'].forEach(function(sig) { From 39289bb8c8f9d82dc1975ace7048fb4ef74fc882 Mon Sep 17 00:00:00 2001 From: Levin Rickert Date: Sun, 30 Jul 2017 17:51:23 +0200 Subject: [PATCH 2/5] Launch PowerShell agent in on middleware-creation --- packages/react-error-overlay/middleware.js | 5 +++++ packages/react-scripts/scripts/start.js | 6 ------ 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/react-error-overlay/middleware.js b/packages/react-error-overlay/middleware.js index 2857eea6023..4e963b3cace 100644 --- a/packages/react-error-overlay/middleware.js +++ b/packages/react-error-overlay/middleware.js @@ -11,6 +11,11 @@ const { launchEditor } = require('react-dev-utils/launchEditor'); module.exports = function createLaunchEditorMiddleware() { + if (process.platform === 'win32') { + const { launchPowerShellAgent } = require('react-dev-utils/launchEditor'); + launchPowerShellAgent(); + } + return function launchEditorMiddleware(req, res, next) { // Keep this in sync with react-error-overlay if (req.url.startsWith('/__open-stack-frame-in-editor')) { diff --git a/packages/react-scripts/scripts/start.js b/packages/react-scripts/scripts/start.js index 8408a848117..b86943b4d9d 100644 --- a/packages/react-scripts/scripts/start.js +++ b/packages/react-scripts/scripts/start.js @@ -85,12 +85,6 @@ choosePort(HOST, DEFAULT_PORT) } console.log(chalk.cyan('Starting the development server...\n')); openBrowser(urls.localUrlForBrowser); - if (process.platform === 'win32') { - const { - launchPowerShellAgent, - } = require('react-dev-utils/launchEditor'); - launchPowerShellAgent(); - } }); ['SIGINT', 'SIGTERM'].forEach(function(sig) { From 55dca23eef8681a2f073b137e1197d21d53708e2 Mon Sep 17 00:00:00 2001 From: Levin Rickert Date: Sun, 30 Jul 2017 17:52:22 +0200 Subject: [PATCH 3/5] Only launch PowerShell agent if no editor is configured --- packages/react-error-overlay/middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-error-overlay/middleware.js b/packages/react-error-overlay/middleware.js index 4e963b3cace..d56139d3d4e 100644 --- a/packages/react-error-overlay/middleware.js +++ b/packages/react-error-overlay/middleware.js @@ -11,7 +11,7 @@ const { launchEditor } = require('react-dev-utils/launchEditor'); module.exports = function createLaunchEditorMiddleware() { - if (process.platform === 'win32') { + if (process.platform === 'win32' && !process.env.REACT_EDITOR) { const { launchPowerShellAgent } = require('react-dev-utils/launchEditor'); launchPowerShellAgent(); } From 5785eaf23b78b3ab0bafe9139977fc026166fa74 Mon Sep 17 00:00:00 2001 From: Levin Rickert Date: Sun, 30 Jul 2017 19:35:15 +0200 Subject: [PATCH 4/5] Fixed issue with Promise-based editor detection and cleaned up code --- packages/react-dev-utils/launchEditor.js | 111 ++++++++++++--------- packages/react-error-overlay/middleware.js | 6 +- 2 files changed, 66 insertions(+), 51 deletions(-) diff --git a/packages/react-dev-utils/launchEditor.js b/packages/react-dev-utils/launchEditor.js index 76cc3e72bb5..1cf14392067 100644 --- a/packages/react-dev-utils/launchEditor.js +++ b/packages/react-dev-utils/launchEditor.js @@ -30,6 +30,14 @@ class PowerShell extends EventEmitter { } ); + 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'); + } + let output = []; this._proc.stdout.on('data', data => { if (data.indexOf(EOI) !== -1) { @@ -39,17 +47,9 @@ class PowerShell extends EventEmitter { 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); @@ -154,63 +154,76 @@ function getArgumentsForLineNumber(editor, fileName, lineNumber, workspace) { } let powerShellAgent = null; -function launchPowerShellAgent() { +function tryLaunchPowerShellAgent() { if (!powerShellAgent) { - powerShellAgent = new PowerShell(); + try { + powerShellAgent = new PowerShell(); + } catch (err) { + // Failed to start, ignore silent... + powerShellAgent = null; + } } } function guessEditor() { - return new Promise(resolve => { - // Explicit config always wins - if (process.env.REACT_EDITOR) { - return resolve(shellQuote.parse(process.env.REACT_EDITOR)); - } - + 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 - try { - if (process.platform === 'darwin') { + if (process.platform === 'darwin') { + 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 resolve([COMMON_EDITORS_OSX[processName]]); + return 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); - - if (COMMON_EDITORS_WIN.indexOf(shortProcessName) !== -1) { - return resolve([fullProcessPath]); - } - } - }); + } catch (error) { + // Ignore... } - } catch (error) { - // Ignore... - } + } 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); - // 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]); + if (COMMON_EDITORS_WIN.indexOf(shortProcessName) !== -1) { + return fullProcessPath; + } + } + }); } + }); +} + +function tryGetEditor() { + // Explicit config always wins + if (process.env.REACT_EDITOR) { + return Promise.resolve(shellQuote.parse(process.env.REACT_EDITOR)); + } - return resolve([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]; + } }); } @@ -252,7 +265,7 @@ function launchEditor(fileName, lineNumber) { return; } - guessEditor().then(([editor, ...args]) => { + tryGetEditor().then(([editor, ...args]) => { if (!editor) { printInstructions(fileName, null); return; @@ -315,5 +328,5 @@ function launchEditor(fileName, lineNumber) { module.exports = { launchEditor, - launchPowerShellAgent, + tryLaunchPowerShellAgent, }; diff --git a/packages/react-error-overlay/middleware.js b/packages/react-error-overlay/middleware.js index d56139d3d4e..b909cc79f1c 100644 --- a/packages/react-error-overlay/middleware.js +++ b/packages/react-error-overlay/middleware.js @@ -12,8 +12,10 @@ const { launchEditor } = require('react-dev-utils/launchEditor'); module.exports = function createLaunchEditorMiddleware() { if (process.platform === 'win32' && !process.env.REACT_EDITOR) { - const { launchPowerShellAgent } = require('react-dev-utils/launchEditor'); - launchPowerShellAgent(); + const { + tryLaunchPowerShellAgent, + } = require('react-dev-utils/launchEditor'); + tryLaunchPowerShellAgent(); } return function launchEditorMiddleware(req, res, next) { From 1a7907ffb3b530221978af2d63dc570829f48625 Mon Sep 17 00:00:00 2001 From: Levin Rickert Date: Thu, 7 Sep 2017 22:11:23 +0200 Subject: [PATCH 5/5] Fixed mapping of PS output to right Promise --- packages/react-dev-utils/errorOverlayMiddleware.js | 5 +---- packages/react-dev-utils/launchEditor.js | 13 ++++++++----- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/react-dev-utils/errorOverlayMiddleware.js b/packages/react-dev-utils/errorOverlayMiddleware.js index b4b8245be95..723b750620c 100644 --- a/packages/react-dev-utils/errorOverlayMiddleware.js +++ b/packages/react-dev-utils/errorOverlayMiddleware.js @@ -8,14 +8,11 @@ */ '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) { - const { - tryLaunchPowerShellAgent, - } = require('react-dev-utils/launchEditor'); tryLaunchPowerShellAgent(); } diff --git a/packages/react-dev-utils/launchEditor.js b/packages/react-dev-utils/launchEditor.js index de418d49be4..e8aff00db98 100644 --- a/packages/react-dev-utils/launchEditor.js +++ b/packages/react-dev-utils/launchEditor.js @@ -38,10 +38,15 @@ class PowerShell extends EventEmitter { 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) { - this.emit('resolve', output.join('')); + const eventName = 'resolve' + ++this._callbackCounter; + this.emit(eventName, output.join('')); output = []; } else { output.push(data); @@ -51,10 +56,8 @@ class PowerShell extends EventEmitter { invoke(cmd) { return new Promise(resolve => { - this.on('resolve', data => { - resolve(data); - this.removeAllListeners('resolve'); - }); + const eventName = 'resolve' + ++this._resolveCounter; + this.once(eventName, resolve); this._proc.stdin.write(cmd); this._proc.stdin.write(os.EOL);