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.

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

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

Merged
merged 3 commits into from
Jun 27, 2017

Conversation

levrik
Copy link
Contributor

@levrik levrik commented Jun 17, 2017

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

@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

@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
@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!

}
}
} 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

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
@gaearon
Copy link
Contributor

gaearon commented Jun 28, 2017

'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...

}
} 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.

romaindso pushed a commit to romaindso/create-react-app that referenced this pull request Jul 10, 2017
* 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
* 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
* 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants