browserify / resolve

Implements the node.js require.resolve() algorithm
MIT License
776 stars 183 forks source link

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

Closed paazmaya closed 5 years ago

paazmaya commented 5 years ago

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 commented 5 years ago

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.

SimonSchick commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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.