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

Auto-detect running editor on Windows for error overlay #2552

Merged
merged 3 commits into from
Jun 27, 2017
Merged

Auto-detect running editor on Windows for error overlay #2552

merged 3 commits into from
Jun 27, 2017

Conversation

levrik
Copy link
Contributor

@levrik levrik commented Jun 17, 2017 •
edited
Loading

I decided to go with an a bit different approach for Windows than on macOS. This resolves the first two issues mentioned below.

The new approach detects only by the file name and re-uses the whole path found in the process list to launch the editor. On macOS this was not possible because some executables did not match the command needed to run (if I understood that right).

Regarding the third one:
We could improve that by launching a PowerShell at the beginning in the background and just pipe the commands in then. This would require to switch to a Promise/callback-based implementation of the whole feature. I would pick that up in a second PR later on if you're okay with it 🙂

Tested on Windows 10 with latest VS Code, Atom and Sublime Text 3.

The original content of this PR:

So, this is a first try to get the auto-detect feature of the editor for the error overlay working on Windows. There are a few issues with the current implementation so I would like to discuss these issues first.

  • At the moment it's only working if the found editor has its executable on PATH. Unfortunately we have drive letters on Windows so we cannot hardcode a path.
  • For example Atom installs itself into the user profile and has one folder per version. The current implemented detection does not work here. Do we need to require the full path for the detection or can we just search by the executable's name?
  • The whole process feels somehow slow. Mostly caused by the PowerShell's startup time (about 500 ms on my system).

Also you can find the previous code here: levrik/create-react-app@adee1d0

2017-06-17_12-25-40

@levrik levrik force-pushed the detect-editor-windows branch from eb22140 to adee1d0 Compare June 17, 2017 10:41
@gaearon
Copy link
Contributor

gaearon commented Jun 17, 2017

Nice, thank you for working on this! Let me know when you feel it's ready for review and I can give this a spin.

@levrik levrik force-pushed the detect-editor-windows branch 2 times, most recently from fc4673c to 837ccae Compare June 17, 2017 13:14
@levrik
Copy link
Contributor Author

levrik commented Jun 17, 2017 •
edited
Loading

@gaearon Ready for a first review :)

Edit: AppVeyor build is broken but it seems to be not related to this PR since other PRs having the same failure.

@levrik levrik changed the title [WIP] Auto-detect running editor on Windows for error overlay Auto-detect running editor on Windows for error overlay Jun 17, 2017
@levrik levrik force-pushed the detect-editor-windows branch from 837ccae to 9196c7b Compare June 17, 2017 13:23
@levrik levrik force-pushed the detect-editor-windows branch from 9196c7b to c0d2600 Compare June 18, 2017 14:45
@Timer Timer mentioned this pull request Jun 18, 2017
Merged
@gaearon
Copy link
Contributor

gaearon commented Jun 27, 2017

We could improve that by launching a PowerShell at the beginning in the background and just pipe the commands in then. This would require to switch to a Promise/callback-based implementation of the whole feature.

This sounds good to try in a separate PR if you'd like!

gaearon
gaearon reviewed Jun 27, 2017
View reviewed changes
packages/react-dev-utils/launchEditor.js Outdated
}
}
} else if (process.platform === 'win32') {
const output = child_process.execSync('powershell -Command "Get-Process | Select-Object Path"').toString();
Copy link
Contributor

@gaearon gaearon Jun 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

What if powershell doesn't exist on the system? Seems like this prints stuff to console. Can we silence it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll push up a fix for this

@gaearon gaearon added this to the 1.0.8 milestone Jun 27, 2017
@gaearon gaearon merged commit 2874e12 into facebook:master Jun 27, 2017
@gaearon
Copy link
Contributor

gaearon commented Jun 27, 2017

This looks great. Thank you so much!

@levrik levrik deleted the detect-editor-windows branch June 27, 2017 15:18
@gaearon gaearon mentioned this pull request Jun 28, 2017
Merged
@gaearon
Copy link
Contributor

gaearon commented Jun 28, 2017

Fixed in 1.0.8. Please verify.
https://github.com/facebookincubator/create-react-app/releases/tag/v1.0.8

Daniel15
Daniel15 reviewed Jun 28, 2017
View reviewed changes
packages/react-dev-utils/launchEditor.js
'atom.exe',
'sublime_text.exe',
'notepad++.exe',
];
Copy link
Member

Choose a reason for hiding this comment

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

What about Visual Studio (devenv.exe)? It's the IDE for Windows. 😛

Copy link
Contributor

Choose a reason for hiding this comment

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

We're taking PRs 😛
VS was my first IDE...

Daniel15
Daniel15 reviewed Jun 28, 2017
View reviewed changes
packages/react-dev-utils/launchEditor.js
}
} else if (process.platform === 'win32') {
const output = child_process
.execSync('powershell -Command "Get-Process | Select-Object Path"', {
Copy link
Member

Choose a reason for hiding this comment

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

It's really annoying that Node.js doesn't have built-in abstractions for this. Hopefully better FFI support should make this more reliable at least, instead of having to parse strings.

@levrik levrik mentioned this pull request Jul 2, 2017
Closed
romaindso pushed a commit to romaindso/create-react-app that referenced this pull request Jul 10, 2017
@levrik @romaindso
Auto-detect running editor on Windows for error overlay (facebook#2552)
ab64dfe 
* Auto-detect running editor on Windows for error overlay

* Ignore process output if powershell call fails

* Support Notepad++
wmonk referenced this pull request in wmonk/create-react-app-typescript Aug 7, 2017
@levrik @wmonk
Auto-detect running editor on Windows for error overlay (#2552)
b9cbf5c 
* Auto-detect running editor on Windows for error overlay

* Ignore process output if powershell call fails

* Support Notepad++
morgs32 pushed a commit to BrickworkSoftware/create-react-app that referenced this pull request Sep 1, 2017
@levrik @morgs32
Auto-detect running editor on Windows for error overlay (facebook#2552)
d35a4aa 
* Auto-detect running editor on Windows for error overlay

* Ignore process output if powershell call fails

* Support Notepad++
@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gaearon gaearon gaearon left review comments

@Daniel15 Daniel15 Daniel15 left review comments

Assignees
No one assigned
Labels
CLA Signed tag: enhancement
Projects
None yet
Milestone
1.0.8
Development

Successfully merging this pull request may close these issues.
4 participants
@levrik @gaearon @Daniel15 @facebook-github-bot