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 loading nvm #101

Merged
merged 1 commit into from Mar 2, 2017
Merged

Fix loading nvm #101

merged 1 commit into from Mar 2, 2017

Conversation

rainboxx
Copy link
Contributor

nvm couldn't be loaded when a specific version of npm was in the path even when a .nvmrc was added to the project root. This change allows to place a file .nvmrc into the project root which will be picked up by husky.

Additionally two apparent bugs are fixed:

  • exists doesn't seem to be a command neither on macOS nor on Linux, might be a typo
  • the test for the file nvm.sh is wrong

@rainboxx
Copy link
Contributor Author

rainboxx commented Feb 24, 2017

It should be discussed whether this is the intended behavior.

On the one hand I'd say that if a .nvmrc is in the project it should also be used before anything else. The current implementation with this PR would load nvm even when there's npm in the PATH and no .nvmrc in the project root. Maybe the logic should be 1) check for .nvmrc, then 2) try to load nvm.

On the other hand it looks like it was me installing node without nvm some time ago and it's not shipped with macOS. So basically it's my fault (except maybe the typos).

I'd still argue that if a project has a .nvmrc it should be used for git hooks.

What is your opinion @typicode? I'm happy to change the logic so that it first checks for a nvmrc file prior to loading nvm.

@typicode
Copy link
Owner

typicode commented Mar 2, 2017

Hi @rainboxx,

Thank you for the PR. Good question, would need to think a little more about this case.
Would you mind changing the PR with only the fix for exists? This way it could be merged now.

`exists` doesn't seem to be a command neither on macOS nor on Linux
@rainboxx
Copy link
Contributor Author

rainboxx commented Mar 2, 2017

@typicode of course. I amended the commit to only fix exists and the parameters.

If you'd like to discuss use cases for loading via nvm even when npm is in the path I'm happy to help.

@typicode
Copy link
Owner

typicode commented Mar 2, 2017

Thank you :)

@typicode typicode merged commit 4b89c52 into typicode:master Mar 2, 2017
@rainboxx rainboxx deleted the patch-1 branch March 2, 2017 13:55
@rainboxx
Copy link
Contributor Author

rainboxx commented Mar 2, 2017

Well, thank you for taking this PR :-).

@rainboxx rainboxx mentioned this pull request Mar 3, 2017
@rainboxx rainboxx mentioned this pull request Mar 21, 2017
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.

None yet

2 participants