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 #1629

Closed
wants to merge 2 commits into from
Closed

Change how npm.cmd is located in NpmHelper #1629

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.

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.
@nikolaia
Copy link
Contributor Author

nikolaia commented Aug 4, 2017

Maybe I should have pointed this against the hotfix_fake4 branch?

@nikolaia
Copy link
Contributor Author

nikolaia commented Aug 4, 2017

@matthid
Copy link
Member

matthid commented Aug 30, 2017

Sorry for the late response. Ideally this PR would include the migration of the NPMModule to a FAKE 5 module (see https:/fsharp/FAKE/blob/master/help/markdown/contributing.md):

Therefore new features need to be at least available as new FAKE 5 module (that might mean that the old module needs to be migrated as part of the PR).

There is also a guide how to do it (https:/fsharp/FAKE/blob/master/help/markdown/contributing.md#porting-a-module-to-fake-5). Do you think you can tackle that? This will ensure this module will still be available on the .netcore (future) version of FAKE.

The code itself makes sense, the package is hopelessly outdated anyway. Question is: Should there be a way for users to provide a fallback/the path to the executable?

@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 Aug 30, 2017
@nikolaia
Copy link
Contributor Author

The code itself makes sense, the package is hopelessly outdated anyway. Question is: Should there be a way for users to provide a fallback/the path to the executable?

They can already do that using the NpmFilePath parameter/option

Ideally this PR would include the migration of the NPMModule to a FAKE 5 module. Do you think you can tackle that?

I'll look into it

@matthid matthid added Fake 5 module required help wanted and removed waiting for author Some information or action was requested and it needs to be addressed. Or a response from author labels Oct 15, 2017
@nikolaia
Copy link
Contributor Author

Solved by #1822

@nikolaia nikolaia closed this Mar 21, 2018
@nikolaia nikolaia deleted the f_npmhelper_npmcmdlocation branch March 21, 2018 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants