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

Add is-installed-globally for auto-detection #114

Merged
merged 2 commits into from Oct 9, 2017

Conversation

timdeschryver
Copy link
Contributor

See #112 (and avajs/ava#1407)

@nexdrew
Copy link
Collaborator

nexdrew commented Jun 28, 2017

These changes LGTM, assuming we consider is-installed-globally@0.1.0 (and global-dirs@0.1.0) to be stable. Based on my testing with Node 8/npm 5 and this commit, I'm not sure we're ready to do that.

@sindresorhus Perhaps you can elaborate a bit (or point us to an issue) on the current problem with npm 5? Looks like npm 5 symlinks "global installs of a local package" into the global root (i.e. result of npm root --global) instead of copying the package there (as it did in npm 4)?

@sindresorhus
Copy link
Member

assuming we consider is-installed-globally@0.1.0 (and global-dirs@0.1.0) to be stable.

It works from my local testing and the tests, but there might be edge-cases I haven't considered. I wouldn't say it's that important though. Worst case, it shows the wrong install flag and we fix it. It already shows the wrong flag when installed locally, so it's not like it would be worse. Happy to keep this open for a little bit so people can try it out though.

Looks like npm 5 symlinks "global installs of a local package" into the global root (i.e. result of npm root --global) instead of copying the package there (as it did in npm 4)?

Yes, I have a todo to open an issue on the npm issue tracker about it.

@sindresorhus
Copy link
Member

@tdeschryver Can you fix the merge conflict?

@timdeschryver
Copy link
Contributor Author

No problemo!

@sindresorhus sindresorhus merged commit 97d0b97 into yeoman:master Oct 9, 2017
@sindresorhus
Copy link
Member

Thanks :)

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

3 participants