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-gyp dependency #144

Closed
wants to merge 7 commits into from
Closed

Conversation

lgeiger
Copy link
Member

@lgeiger lgeiger commented Dec 9, 2016

This PR brings down the size of node_modules from 10.9MB to 1.3MB.

Fixes #143

@@ -163,7 +170,6 @@ prebuild [options]
--compile -c (compile your project using node-gyp)
--no-compile (skip compile fallback when downloading)
--libc (use provided libc rather than system default)
--backend (specify build backend, default is 'node-gyp')
Copy link
Collaborator

Choose a reason for hiding this comment

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

this option will have to stay, for ninja support

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess if we want to keep ninja we need to include it as a dependency again since it isn't bundled with npm.
But ninja requires node-gyp so we can't remove it and won't gain any improvement in package size.

Copy link
Member Author

Choose a reason for hiding this comment

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

But it would work if the user has node-ninja installed or explicitly included as a dependency of the native module.

I'll add support for it after #145 is merged so I can verify it works.

@@ -1,5 +1,5 @@
var fs = require('fs')
var async = require('async')
var waterfall = require('async-waterfall')
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

@lgeiger lgeiger force-pushed the npm-rebuild branch 2 times, most recently from 2fe6d65 to 96ca343 Compare December 10, 2016 20:39
@lgeiger
Copy link
Member Author

lgeiger commented Dec 10, 2016

The node-ninja build fails on Node 0.12. I don't think this is a problem of prebuild.
I'm going to disable the integration test on Node 0.12.

@@ -0,0 +1,3 @@
node_modules
npm-debug.log
test/
Copy link
Member

Choose a reason for hiding this comment

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

👍

"prebuild:ninja": "../../bin.js --prebuild 7.2.1 --backend node-ninja"
},
"dependencies": {
"node-ninja": "^1.0.2"
}
Copy link
Member

Choose a reason for hiding this comment

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

Awesome!

@lgeiger
Copy link
Member Author

lgeiger commented Dec 12, 2016

I discovered prebuild-install in #148 which will fix my package size concerns.
Hence this PR isn't essential for me anymore. That is to say: If you prefer to keep node-gyp as a direct dependency, I'm happy to extract the integration test and the other minor improvements into a separate PR.

@ralphtheninja
Copy link
Member

@lgeiger I'm cleaning up a bit. Since prebuild-install was created to address this issue (i.e. big dependency tree) I'm closing it.

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.

3 participants