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

Add windows logic to guess the users preferred editor for launchEditor #1216

Merged
merged 8 commits into from
Aug 5, 2020

Conversation

acoates-ms
Copy link
Contributor

@acoates-ms acoates-ms commented Jul 8, 2020

Summary:

There is logic for guessing an appropriate text editor on macOS. This adds similar logic for windows. Without this, developers have to manually set the REACT_EDITOR environment variable in order to navigate to code from redbox/logbox.

This change roughly aligns with the logic on macOS. If REACT_EDITOR is set we default to using that. But if the developer is running in order VSCode, SublimeText, Visual Studio, or lastly notepad, then we assume that we should use that editor.

Also fixed an issue where if the linenumber wasn't specified, the workspace would not be passed to vscode, which could cause the file to open in the incorrect vscode window.

Test Plan:

Tested locally on windows with these apps running / not running or REACT_EDITOR set, and clicking on a stack from within a logbox.

packages/tools/src/launchEditor.ts Outdated Show resolved Hide resolved
packages/tools/src/launchEditor.ts Outdated Show resolved Hide resolved
packages/tools/src/launchEditor.ts Show resolved Hide resolved
Comment on lines +144 to +156
const output = execSync(
'tasklist /NH /FO CSV /FI "SESSIONNAME ne Services"',
).toString();

const runningProcesses = output
.split('\n')
.map(line => line.replace(/^"|".*\r$/gm, ''));
const processNames = Object.keys(COMMON_WINDOWS_EDITORS);
for (const processName of processNames) {
if (runningProcesses.includes(processName)) {
return [COMMON_WINDOWS_EDITORS[processName]];
}
}
Copy link
Member

Choose a reason for hiding this comment

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

makes sense to reuse this logic from #1222 maybe?

Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

I wonder if there's an npm package that would handle this heavy lifting for us :D

@alloy alloy merged commit 6adda94 into react-native-community:master Aug 5, 2020
@alloy
Copy link
Member

alloy commented Aug 5, 2020

Going ahead and merge this 👍

thymikee added a commit that referenced this pull request Sep 17, 2020
* origin/master: (81 commits)
  chore(deps): bump node-fetch from 2.6.0 to 2.6.1 (#1267)
  v4.13.0
  chore: remove await in getLatestFile function call from downloadProfile file. (#1266)
  Augment react-native info with out of tree platform packages info (#1264)
  feat: add a command to download the Hermes Sampling Profiler to a local machine (#1246)
  chore(deps): bump lodash from 4.17.15 to 4.17.19 (#1221)
  Fix package name for main activity when appId set (#1242)
  Just use the users default browser (#1257)
  feat: improve progress indicator for iOS builds without xcpretty (#1254)
  fix: 🐛 executableFolderPath to run app name in iOS 14.0 (#1236)
  chore(deps): bump elliptic from 6.5.2 to 6.5.3 (#1233)
  v4.12.0
  Add Windows info to react-native info (#1227)
  add new version RN Podfile link, autolinking docs. (#1245)
  docs: add linkage instructions for `yarn` v2 (#1237)
  v4.11.0
  fix: adjust range on react-native peer dep to support 0.63 (#1234)
  chore: update jest config (#1241)
  Use new chrome based edge if the user is already running it, and not running chrome (#1222)
  Add windows logic to guess the users preferred editor for launchEditor (#1216)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants