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

Change how npm.cmd is located in NpmHelper [hotfix_fake4] #1630

Closed
wants to merge 2 commits into from
Closed

Change how npm.cmd is located in NpmHelper [hotfix_fake4] #1630

wants to merge 2 commits into from

Conversation

nikolaia
Copy link
Contributor

@nikolaia nikolaia commented Aug 4, 2017

I've noticed that Yarn adds nodejs\bin to the path variable, which in the old code could return a folder that did not contain the npm.cmd file. I fixed this and also removed the fallback to the npm.js package, as it is silly to assume where the package directory is. If people are using that package they should overwrite the NpmFilePath variable.

This is the same as #1629 just against the hotfix_fake4 branch

I've noticed that Yarn adds `nodejs\bin` to the path variable, which would
return in the old method used to find npm.cmd, but not actually contain
the npm.cmd file. I also removed the fallback to the npm.js package, as it
is silly to assume where the package directory is. If people are using
that package they should overwrite the NpmFilePath variable.
@matthid
Copy link
Member

matthid commented Aug 18, 2017

Is this a hotfix?

Note that the fallback to "./packages/Npm.js/tools/npm.cmd" assumes that one uses the https://www.nuget.org/packages/Npm.js package in combination with Paket.
It looks a bit outdated?

@nikolaia
Copy link
Contributor Author

The old way was a bit naive and broke in a lot of situation, the most noticeable one being that it would fail if you have a value containing nodejs in your PATH env variable that didn't contain npm.cmd. An example of that could be C:\Program FIles\nodejs\bin - while npm.cmd is located in C:\Program FIles\nodejs that comes later in your PATH env var.

The fallback to the package, as you say, assumes that you actuallyt use the package together with Paket. The package seems outdated and it just makes for an unclear error message.

@matthid matthid added the waiting for author Some information or action was requested and it needs to be addressed. Or a response from author label Oct 15, 2017
@nikolaia
Copy link
Contributor Author

Did you not consider this to be a hotfix for FAKE4? The dependency on/fallback to the outdated Npm.js nuget package can probably be considered broken, and this PR changes it to look more thorough trough the PATH variable for npm.exe. Basically same behaviour as on *nix where it assumes you install NPM yourself.

@nikolaia
Copy link
Contributor Author

Solved for FAKE5 in #1822

@matthid
Copy link
Member

matthid commented Apr 26, 2018

Did you not consider this to be a hotfix for FAKE4?

Problem is that I cannot release fake 4 for some reason. Also at this time I don't think there is a good reason to stay on fake 4 anymore. The first step of the migration to fake 5 (upgrading the nuget package to v5) should be non-breaking.

@matthid matthid removed the waiting for author Some information or action was requested and it needs to be addressed. Or a response from author label Apr 26, 2018
@nikolaia nikolaia closed this Apr 28, 2018
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.

2 participants