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: download prebuilt binaries when installing with pnpm #66

Merged
merged 1 commit into from
Feb 8, 2018

Conversation

zkochan
Copy link
Contributor

@zkochan zkochan commented Feb 5, 2018

Like Yarn, pnpm does not add a _from field to the package.json files of the installed dependencies. So pnpm needs the same workaround as Yarn

@zkochan
Copy link
Contributor Author

zkochan commented Feb 5, 2018

A better solution to detect the running package manager might be to use which-pm-runs. It gets the package manager info from process.env.npm_config_user_agent. It can correctly identify npm/pnpm and Yarn

@ralphtheninja
Copy link
Member

A better solution to detect the running package manager might be to use which-pm-runs. It gets the package manager info from process.env.npm_config_user_agent. It can correctly identify npm/pnpm and Yarn

We could use it if you like and think it's the preferred way of doing it.

@zkochan
Copy link
Contributor Author

zkochan commented Feb 6, 2018

I updated the code. I think it is more reliable because sometimes a package manager can be installed via an alias. Like npm has npmc for instance, so the execPath might not contain the package manager name.

However, that's probably a rare situation right now. I am fine with both solutions.

@ralphtheninja
Copy link
Member

@zkochan I prefer using your module and also prefer inviting you to prebuild org if you're cool with that

bin.js Outdated

if (util.isYarnPath(execPath) && /node_modules/.test(process.cwd())) {
try {
var pm = whichPmRuns()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If whichPmRuns() threw an error instead of returning undefined this could be

try {
  isNpm = whichPmRuns().name === 'npm'
} catch (err) { }

@zkochan WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I checked which-pm-runs and it won't ever fail, so I simplified the code by removing the redundant try/catch

@zkochan
Copy link
Contributor Author

zkochan commented Feb 6, 2018

@ralphtheninja awesome! I am cool with it

@ralphtheninja
Copy link
Member

@ralphtheninja awesome! I am cool with it

Nice! Invitation sent. Sorry for the delay, was traveling yesterday.

@ralphtheninja
Copy link
Member

Just need at least one more maintainer to have a look and then we :shipit:

@ralphtheninja ralphtheninja merged commit a65b3ba into prebuild:master Feb 8, 2018
@ralphtheninja
Copy link
Member

2.5.1

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