Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
No more symlinks on Windows #1552
No more symlinks on Windows #1552
Changes from all commits
eaaefdd
d291675
a6d760f
b346279
7e2b890
d8254a4
d4980a9
ad05c22
35eb26c
081e2b3
b5b1fc7
7e38f86
efcb892
2906867
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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'm still a little unsure about making this the behavior for all platforms. It's relatively minor, but it is another indirection and since Rust
std::process::Command
doesn't useexec
, it may cause unexpected issues on top of any small performance hit.One that comes to mind is signals (e.g. pressing
Ctrl-C
in the terminal while a process is running). We don't handle them perfectly right now, but I know a lot of work went into making sure thatCtrl-C
would cancel the underlying Node process (as a user would expect), not only the wrapper process.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.
Hi Charles!
Thank you for the feedback.
So you prefer this change is changed only for Windows, right? We can make this condition, no problem.
About the
volta run
and.cmd
idea, let me know if you prefer that approach and I'll try to implement 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.
Yeah, if we move forward here, we should limit the scope to only for Windows, so that we have the smallest possible blast radius.
For the
volta run
idea, I want to do a quick prototype to see if it's feasible, then we can look at which approach makes the most sense.And thank you again for persisting with this change, despite our busy schedules making the lead times really long! It will be a big improvement in Windows behavior when we don't need the "Developer Mode" warning 💥
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.
Great! So I'll wait for you before making both changes.