minimistjs / minimist

parse argument options
MIT License
515 stars 30 forks source link

npm package includes non-production files #23

Closed shadowspawn closed 1 year ago

shadowspawn commented 1 year ago

I noticed today that the recent releases include example and test files. The package.json does not include a files property. Not sure how the old releases were done to skip those!

https://packagephobia.com/result?p=minimist

temp % npm init -y
...

temp % npm install minimist

added 1 package, and audited 2 packages in 589ms

1 package is looking for funding
  run `npm fund` for details

found 0 vulnerabilities

temp % ls -R node_modules 
minimist

node_modules/minimist:
CHANGELOG.md    LICENSE     README.md   example     index.js    package.json    test

node_modules/minimist/example:
parse.js

node_modules/minimist/test:
all_bool.js     default_bool.js     long.js         parse_modified.js   stop_early.js
bool.js         dotted.js       num.js          proto.js        unknown.js
dash.js         kv_short.js     parse.js        short.js        whitespace.js
ljharb commented 1 year ago

In general, every package i maintain ships these on purpose - npm explore foo && npm install && npm test should always work.

However, if previous versions in the same major line didn’t include them, it’d be fine to add them to the npmignore config.

silverwind commented 1 year ago

In general, every package i maintain ships these on purpose - npm explore foo && npm install && npm test should always work.

I'd ask you to reconsider. Packages that ship tests are part of the problem why node_modules size explodes for the average project, which reflects badly on the ecosystem, not to mention it slows down install performance.

ljharb commented 1 year ago

@silverwind that's a problem npm could solve holistically by providing a way for a package to ship multiple variants, eg "default" and "with tests", rather than trying to convince individual maintainers to do it.

silverwind commented 1 year ago

I'd suggest to at least use the files property in package.json to allowlist the files you intend to ship.

ljharb commented 1 year ago

We're already set up to use npmignore, and the files property is very dangerous and should never be used - the failure mode of "consumers are broken" is far, far worse than the failure mode of "extra files are included".

shadowspawn commented 1 year ago

Not sure how the old releases were done to skip those!

One mystery solved. When I looked more carefully at difference between 1.2.6 and 1.2.7, the old package did have example/ and test/!

The size difference is largely due to the inclusion of a CHANGELOG. There are also small potentially surplus files with .eslintrc and .nycrc.

I am going to close this, as my initial concern was we had started including example and test files.