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

THRIFT-3485 don't publish extra files to npm #741

Closed
wants to merge 1 commit into from

Conversation

azylman
Copy link

@azylman azylman commented Dec 11, 2015

The thrift npm package is as big as all of our other node dependencies combined. It currently publishes the entire thrift repository, which is ~30mb right now. As far as I can tell, it only requires lib/nodejs, which is 376k.

I propose adding an .npmignore file. You can more info in the npm documentation: https://docs.npmjs.com/misc/developers#keeping-files-out-of-your-package

If the package only requires lib/nodejs, we should be able to add an entry to the npmignore file for every folder (including lib), and then a negated entry for lib/nodejs. We'll also want to include everything from gitignore, because npm will no longer use that once an npmignore file is added.

I've tested this using the method recommended in the npmjs documented, basically installing the package globally and making sure it still requires successfully:

$ npm install -g .
$ node
> var thrift = require('thrift');
undefined
> thrift
{ Thrift:
   { Type:
  ...etc...
$ du -h -d 1 .
236K    ./lib
812K    ./node_modules
1.2M    .

@nsuke
Copy link
Member

nsuke commented Dec 13, 2015

It seems it's already fixed by f264884.
@azylman could you confirm ?

@azylman
Copy link
Author

azylman commented Dec 13, 2015

@nsuke Is that commit published at all? I'm still seeing this behavior in the latest version published to npm (0.9.3)

@azylman
Copy link
Author

azylman commented Dec 13, 2015

@nsuke Looks like that's not published in 0.9.3. I just confirmed that master does not have this problem, making this PR unnecessary. Hopefully that change can get published soon :/

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.

2 participants