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

Revert "windows: Add preliminary WSL support for npm and npx" #527

Closed
wants to merge 1 commit into from

Conversation

craigloewen-msft
Copy link
Contributor

This reverts commit 3471d52.

What / Why

From my understanding, this change enables npm to detect that it is running inside of WSL, and instead of running the Linux version of npm, it will run the Windows installed version of npm. Would you be able to revert this change?

The reasoning behind this is that this could cause performance and compatibility issues for npm. Users in WSL would be expecting to use the Linux version of npm, and instead routing the command to target the Windows version of npm could cause unexpected problems.

I'm happy to dive into more detail on the reasons why, and thank you for thinking about WSL when you made this change! :)

@craigloewen-msft craigloewen-msft requested a review from a team as a code owner November 27, 2019 01:41
@craigloewen-msft
Copy link
Contributor Author

Tagging @infinnie so this is visible to you as you're the author of the original commit!

@infinnie
Copy link
Contributor

infinnie commented Nov 27, 2019

Thanks but no, it did not prevent NPM for Linux from being run.

I had wished that it could fall back to the Windows command prompt when the Linux executable is not found. When the corresponding Linux version exists, however, it would be in a position within $path before the Windows one and get executed instead.

This, if implemented, would prevent NPM for Windows from running into an error when executed inside WSL.

@darcyclarke darcyclarke added Agenda will be discussed at the Open RFC call Needs Discussion is pending a discussion labels Nov 27, 2019
@darcyclarke
Copy link
Contributor

@craigloewen-msft @infinnie I've added this PR to the agenda of our Open RFC call happening later today; I encourage you both to join if you'd like to discuss the ideal solution moving forward: npm/rfcs#70

@craigloewen-msft
Copy link
Contributor Author

Thanks! I'll join the call.

@craigloewen-msft
Copy link
Contributor Author

For more context, if I run these commands using the Linux binaries targeting a project in my Linux file system in WSL, it succeeds:
image

However, if I use the Windows version of npm to target the same project in my Linux file system, I receive an error since CMD is unable to access these Linux files.
image

Thanks for your help!

@darcyclarke darcyclarke added Release 6.x work is associated with a specific npm 6 release Bug thing that needs fixing and removed Agenda will be discussed at the Open RFC call Needs Discussion is pending a discussion labels Nov 28, 2019
@darcyclarke darcyclarke added this to the Release 6.14.0 milestone Nov 28, 2019
@infinnie
Copy link
Contributor

infinnie commented Nov 29, 2019

For more context, if I run these commands using the Linux binaries targeting a project in my Linux file system in WSL, it succeeds:
image

However, if I use the Windows version of npm to target the same project in my Linux file system, I receive an error since CMD is unable to access these Linux files.
image

Thanks for your help!

Hello.

It was not meant to replace the Linux version. Just in case the Linux one is not installed and Windows files are what need to be handled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Release 6.x work is associated with a specific npm 6 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants