-
-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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.
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
Speed up editor detection on Windows #2711
Conversation
e8f2be6
to
e0fefca
Compare
It's fine to do on start if it's not blocking the first compilation. |
@gaearon Can you give me some hint where to put that best? |
In success callback of start.js maybe? |
5a86215
to
5add09c
Compare
35e7f29
to
30b2293
Compare
before ~600ms after ~300ms
30b2293
to
cd28840
Compare
@gaearon Ready for review. |
@@ -85,6 +85,12 @@ choosePort(HOST, DEFAULT_PORT) | |||
} | |||
console.log(chalk.cyan('Starting the development server...\n')); | |||
openBrowser(urls.localUrlForBrowser); | |||
if (process.platform === 'win32') { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@gaearon Friendly ping 🙂 |
I'm not very happy with win32-specific code in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this part of the middleware code, and not add specific calls to start.js
.
@gaearon Okay. Moved it into |
@@ -11,6 +11,11 @@ | |||
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this ever fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fails silent if powershell.exe
was not found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's call it tryLaunchPowerShellAgent
to make it clear it's always safe to call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or wait. Just tested it again. It seems to break the display of the information about the editor integration if powershell.exe
is not found. I look into that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Fixed.
b46e366
to
5785eaf
Compare
I'll try to review next week. Please ping me on Wednesday. |
@gaearon Friendly Wednesday ping. Just realized that you meant the last Wednesday und not this. Somehow got confused there. Sorry. |
return new Promise(resolve => { | ||
this.on('resolve', data => { | ||
resolve(data); | ||
this.removeAllListeners('resolve'); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed 🙂
@levrik Friendly ping back :-) |
@gaearon Sorry, had a busy week and forgot about this. |
2e0b738
to
2a0560b
Compare
2a0560b
to
1a7907f
Compare
Is this ready for another review? |
@gaearon Yes. Totally! |
const os = require('os'); | ||
const chalk = require('chalk'); | ||
const shellQuote = require('shell-quote'); | ||
|
||
// Inspired by https://github.com/rannn505/node-powershell |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
|
||
invoke(cmd) { | ||
return new Promise(resolve => { | ||
const eventName = 'resolve' + ++this._resolveCounter; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
@gaearon Sorry that nothing happened here anymore. Quick question: Would you like to get these improvements in? I'm not sure if it's worth it. |
Yes, totally! |
Closed in favor of #3808 |
Follow up to #2552
Time measurement for getting process list (on my machine): ~300ms
One question: Should we launch the PowerShell on the first click in the error overlay (current implementation) or directly on server start (first click would be faster already then)?