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

Fix launching text editor from a redbox stacktrace on windows #5238

Closed

Conversation

janicduplessis
Copy link
Contributor

Launch the editor with cmd on windows.

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @frantic, @pcottle and @sahrens to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jan 10, 2016
@pcottle
Copy link
Contributor

pcottle commented Jan 10, 2016

I'm worried the args.unshift will be brittle if someone else changes the implementation above.

maybe we should just fork on win32 earlier? we already have getArgumentsForLineNumber(editor, fileName, lineNumber) abstracted out. id just like to see something like:

args = [
  editor,
  '\c', // explanation why this is
  fileName,
  lineNumber ? getArgumentsForLineNumber(editor, fileName, lineNumber) : '',
];

@janicduplessis
Copy link
Contributor Author

@pcottle The goal here is just to launch the editor in a shell because Windows can only run .exe files with child_process.spawn and atom for example, uses a .cmd file. I think forking at child_process.spawn is fine but you are right, using unshift isn't a good idea because it will modify the args object if we ever change something after it could break.

There is an open pull request in node at the moment to add a shell option to spawn that could be better to use eventually. nodejs/node#4598

@facebook-github-bot
Copy link
Contributor

@janicduplessis updated the pull request.

@facebook-github-bot
Copy link
Contributor

@janicduplessis updated the pull request.

@pcottle
Copy link
Contributor

pcottle commented Jan 11, 2016

Great! A ton cleaner. will import

@pcottle
Copy link
Contributor

pcottle commented Jan 11, 2016

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/797876773651420/int_phab to review.

@ghost ghost closed this in e08a7f3 Jan 11, 2016
@janicduplessis janicduplessis deleted the launch-editor-windows branch January 12, 2016 01:00
christopherdro pushed a commit to wildlifela/react-native that referenced this pull request Jan 20, 2016
Summary:
Launch the editor with cmd on windows.
Closes facebook#5238

Reviewed By: svcscm

Differential Revision: D2819943

Pulled By: pcottle

fb-gh-sync-id: a38f88bb9be72871cc3a37367973371176799d9e
grabbou pushed a commit to react-native-community/cli that referenced this pull request Sep 26, 2018
Summary:
Launch the editor with cmd on windows.
Closes facebook/react-native#5238

Reviewed By: svcscm

Differential Revision: D2819943

Pulled By: pcottle

fb-gh-sync-id: a38f88bb9be72871cc3a37367973371176799d9e
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants