justmoon / node-extend

Simple function to extend objects
MIT License
341 stars 68 forks source link

Specify files to publish to npm #53

Closed stefnotch closed 4 years ago

stefnotch commented 4 years ago

Currently a number of not necessary files are being published to npm. This includes files such as .editorconfig and .travis.yml. Considering that they make up more than half of the filesize of this package, it would be worthwhile to not publish them. This PR does this using the files field in the package.json.

I've chosen to use the files field instead of the .npmignore, because the latter is more of a whack a mole and has to be kept in sync with the .gitignore.

ljharb commented 4 years ago

Separately, disk space isn't that important; what matters is the final size in the bundle you send to browsers. Files you never require don't matter.

stefnotch commented 4 years ago

That is true, however, not publishing such files has advantages in other use cases such as when running a CI pipeline. A CI pipeline generally works with a clean environment and executes npm install every time. This causes it to download the package over and over again.

ljharb commented 4 years ago

A CI pipeline should be running npm ci for an app, since apps should have lockfiles; additionally the npm cache is often persisted across runs, meaning it doesn't have to be redownloaded. Either way, they're all tarballed, and the delta in zipped download size is negligible.

stefnotch commented 4 years ago

Most npm packages still exclude the test and whatnot folders, especially once the package size grows. Otherwise you end up with packages that add multiple MBs to the download, making them more inconvenient to use. Especially considering that in many regions the download speeds are quite limited. For small packages, I guess it's fine when the small test folder is included.

stefnotch commented 4 years ago

Out of curiosity, why is is this the case? I've never heard anything about this, but I'm always happy to learn something new.

Thanks - I didn't realize test was ignored here; it shouldn't be, because npm explore foo && npm install && npm test should always pass.

ljharb commented 4 years ago

Many do; but all 250+ of the packages I maintain include all their tests, as one data point.

This is an expectation that dates from the early days of node and npm; surely many newcomers to the ecosystem are unaware of it, but it should remain true - particularly because you should not be required to have 1) an internet connection, and 2) a link to a github repo that might not last forever, to be able to run your dependencies' tests. The only immutable thing is a package; that should be the source of truth.

stefnotch commented 4 years ago

The top Stackoverflow answers ( https://stackoverflow.com/questions/25124844/should-i-npmignore-my-tests and https://stackoverflow.com/questions/8617753/exclude-test-code-in-npm-package ) say something else. Would you mind adding your stance there then?

That the npm documentation itself is really vague about such things doesn't help either.

For the record, the most popular packages such as chalk, react and lodash all exclude the tests.

stefnotch commented 4 years ago

But, as your stance is a quite valid one, I'll accept it and close this PR :)