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

Use fs.realpath to handle nvm symlink proxy #1

Merged
merged 1 commit into from
Dec 16, 2017
Merged

Use fs.realpath to handle nvm symlink proxy #1

merged 1 commit into from
Dec 16, 2017

Conversation

zikaari
Copy link
Contributor

@zikaari zikaari commented Dec 15, 2017

At least on windows (not sure of other platforms), nvm, will use symlink proxying to make things work and look natural.

But since symlink lives somewhere else on file system, __dirname !~= 'C:\Program Files\nodejs...' but more so __dirname ~= 'C:\Users\...\AppData\nvm\v7.0.0\...'

@sindresorhus
Copy link
Owner

Wouldn't it make more sense to do fs.realPath in https://github.com/sindresorhus/global-dirs instead?

@zikaari
Copy link
Contributor Author

zikaari commented Dec 16, 2017

That was my initial plan as well, but then I dropped it. The primary reason being that use cases of global-dirs are very diverse, furthermore:

  1. It'll be most likely a breaking change
  2. If a global-dirs consumer needs "actual" path, they can do fs.realpath themselves, however there is no way of going other way around, imagine another consumer who doesn't want the "actual" path of global node_modules but instead the "typical" one (C:\Program Files\nodejs...). Introducing this change will break their expectations of what they'll get.

Also, I think real path of C:\Program Files\nodejs\... is kind of analogous to private API, subject to weird behaviors at any time.

Instead, maybe this situation/caveat can be explained in global-dirs docs.

Also, what exactly is wrong with npm@5.x that's leading to failing tests?

@sindresorhus
Copy link
Owner

That's a very good point. Thanks for elaborating. I agree.

@sindresorhus
Copy link
Owner

Also, what exactly is wrong with npm@5.x that's leading to failing tests?

See: yeoman/update-notifier#114 (comment)

@sindresorhus sindresorhus merged commit b898cd4 into sindresorhus:master Dec 16, 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