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

Reduce the size of the npm package by limiting the included files #171

Closed
paazmaya opened this issue Oct 31, 2018 · 5 comments
Closed

Reduce the size of the npm package by limiting the included files #171

paazmaya opened this issue Oct 31, 2018 · 5 comments

Comments

@paazmaya
Copy link

Looks like the files property (https://docs.npmjs.com/files/package.json#files) is not used in package.json to specify the included files, nor is the .npmignore file (https://docs.npmjs.com/misc/developers#keeping-files-out-of-your-package) is being used for blacklisting unwanted files, for the package published to npm.

Would you consider adding either the files property or the .npmignore file, so that the resulting package file would have smaller size?

The current size can be seen when executing the command npm pack (https://docs.npmjs.com/cli/pack).

This issue was create via tawata

@ljharb
Copy link
Member

ljharb commented Oct 31, 2018

No. The files property is incredibly dangerous and should never be used; and the size of the installed package is irrelevant.

Please do not spam issues in this fashion.

@ljharb ljharb closed this as completed Oct 31, 2018
@SimonSchick
Copy link

SimonSchick commented Nov 13, 2018

@ljharb Can you elaborate how files in incredibly dangerous?
Most people say the exact opposite as files is _opt-inshipping rather thanopt-out`.
I'm legitimately curious to hear the reasoning behind this.

Also I don't think the package size is irrelevant, especially when your package consist of ~90% (by disk usage) or 60% (by byte size) redundant files:

image
(file sizes are rounded as displayed by ncdu)

While I understand that @paazmaya issue was automatically created and this might be annoying to some degree being so dismissive about constructive (albeit automated) criticism seems be in conflict with https://github.com/browserify/browserify/blob/master/code-of-conduct.md#our-standards

@ljharb
Copy link
Member

ljharb commented Nov 13, 2018

@SimonSchick if you use an exclusion list (like "npmignore"), the worst consequence is that you include an unnecessary file. If you use an inclusion list (like "files"), the worst consequence is that you fail to include a necessary file, breaking all of your downstream consumers.

Disk space is effectively infinite and free, as is bandwidth; breakage is infinitely expensive.

I don't agree that I'm being dismissive nor that I violated any part of that CoC; I simply answered the question in the OP, and closed the PR, along with providing a bit more explanation.

@SimonSchick
Copy link

SimonSchick commented Nov 13, 2018

Thanks explaining your reasoning, this makes a little more sense now.

Some food for thought tho:

the worst consequence is that you include an unnecessary file

What if that unnecessary file happens to contain credentials or any other sensitive information?

While this might not apply to this specific project and is relatively unlikely to happen when done via CI/CD, I'm not sure if I would rather risk breaking downstream consumers or potentially leak credentials and, depending on the nature of those credentials, endanger downstream consumers and or customers.

Relevant article: https://medium.com/@jdxcode/for-the-love-of-god-dont-use-npmignore-f93c08909d8d

Anyways, this topic is headed out of scope of this project, feel free to follow up by email if interested.

@ljharb
Copy link
Member

ljharb commented Nov 13, 2018

Credentials that live in an accessible file are going to be leaked at some point no matter what you do - that’s what env vars and environment-specific files are for. If credentials leak, the easy solution is to rotate them - and since you surely have multiple factors (2FA on npm and GitHub, for example; vpn for internal services; etc), leaking one credential shouldn’t be a disaster.

ljharb added a commit that referenced this issue Dec 23, 2019
  - [Breaking] make `preserveSymlinks` false by default (#135)
  - [Breaking] `packageFilter`: bring sync/async into alignment; pass three args: pkg, pkgfile, dir (#171)
  - [Breaking] add `isDirectory`; error early if provided `basedir` is not a directory (#154)
ljharb added a commit to L8D/node-resolve-1 that referenced this issue Jan 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants