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

Remove node-pre-gyp as a bundled dependency and use preinstall script instead #753

Closed
bminer opened this issue Apr 13, 2016 · 13 comments · Fixed by #759
Closed

Remove node-pre-gyp as a bundled dependency and use preinstall script instead #753

bminer opened this issue Apr 13, 2016 · 13 comments · Fixed by #759

Comments

@bminer
Copy link

bminer commented Apr 13, 2016

According to the new docs here, it might be considered better practice to add preinstall script npm install node-pre-gyp instead of using the bundledDependencies key. This would allow other NPM modules to re-use the same version of node-pre-gyp, for example.

See the node-pre-gyp Github repo page for more info. This issue might also be relevant: mapbox/node-pre-gyp#162

@reconbot
Copy link
Member

If it works it work, I'm for it

@reconbot
Copy link
Member

It works, I'll do a patch release in a little bit so you can use it.

@reconbot reconbot reopened this Apr 25, 2016
@reconbot
Copy link
Member

This doesn't work, it's fine when you're installing serialport from a clone, but as a dependency it breaks everything and ends up in an endless loop. I have to revert this change. I've reopened the issue if you want to see if you can give it a go.

reconbot added a commit that referenced this issue Apr 25, 2016
@springmeyer
Copy link

Sorry you hit problems. As per mapbox/node-pre-gyp#162 (comment) I've ask for help to know how to replicate to dig into this.

This would allow other NPM modules to re-use the same version of node-pre-gyp, for example.

Incorrect. They still will install their own version (unless -g is passed). And this is needed since each module needs node-pre-gyp around before installing to install itself.

@reconbot
Copy link
Member

I had this happening using node v4.4.3 (npm v3.8.6) we support node 0.10-5.x so it's bound to be on other platforms too

@springmeyer
Copy link

I'm unable to replicate (mapbox/node-pre-gyp#162 (comment)) so I am presuming this is not a widespread problem and rather something specific to your setup.

@reconbot
Copy link
Member

It's happening to me on two different computers both with NVM. @bminer do you mind hopping on the mapbox thread and try to reproduce?

@bminer
Copy link
Author

bminer commented May 2, 2016

Hmm... I can't replicate either... what version of node-serialport exhibits this problem?

And, I'm really confused on what the issue is exactly... you're saying that whenever you NPM install a package that depends on node-serialport, the preinstall script for node-serialport (which simply installs node-pre-gyp) runs in an infinite loop? Also, why is "node-pre-gyp-github" required as a preinstall dep?

Thanks!

@reconbot
Copy link
Member

reconbot commented May 4, 2016

It was a mistake, I've got a branch with the unbundled package.json without pregyp github. https://github.com/voodootikigod/node-serialport/compare/unbundle npm install voodootikigod/node-serialport#unbundle installs it and it's working for me now...

@reconbot
Copy link
Member

reconbot commented May 4, 2016

But not everywhere. I updated the node-pregyp issue with details. mapbox/node-pre-gyp#162 (comment)

@reconbot
Copy link
Member

reconbot commented May 4, 2016

Keeping this link here for our records, npm/npm#12583 is preventing us from doing this

@reconbot
Copy link
Member

reconbot commented May 6, 2016

Or maybe mapbox/node-pre-gyp#162 (comment)

@reconbot
Copy link
Member

reconbot commented May 6, 2016

@bminer Please test serialport@3.1.2-beta1 I've removed the bundling, I want to make sure it fixes your use case.

@reconbot reconbot closed this as completed May 6, 2016
@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants