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

chore: specify files field in package.json #73

Closed
wants to merge 1 commit into from

Conversation

kazushisan
Copy link

currently the install size is 53.2kB https://packagephobia.com/result?p=minimist

The size is unnecessarily large because the test files, examples and CHANGELOG are all published to npm. This patch will set the files field in package.json to only include the necessary files when publishing. It should reduce the install size to around 12kB

image

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests are very necessarily published to npm; npm explore foo && npm install && npm test should always work.

Separately, the files field is dangerous and should never be used - the proper way to control what gets published is an npmignore file.

@ljharb ljharb marked this pull request as draft December 12, 2024 16:35
@kazushisan
Copy link
Author

kazushisan commented Dec 13, 2024

Thank you for your prompt review!

The tests are very necessarily published to npm; npm explore foo && npm install && npm test should always work.

  • If you were referring to npm install foo && cd node_modules/foo && npm test and were concerned about tests not working when the package is installed as a dependency, it already doesn't work because the test command relies on a dev dependency. That said, I’m not sure if this is a scenario that users expect to work.
  • If you meant npm explore foo && git clone path/to/foo.git && npm install && npm test, tests will work regardless of the publish config.

Given that the current configuration increases the package size by 30kB for every dependency relying on minimist, I believe this change could be beneficial. I hope you're open to discussing this further!

@kazushisan kazushisan requested a review from ljharb December 13, 2024 03:16
@ljharb
Copy link
Member

ljharb commented Dec 13, 2024

You misread; after npm explore, a bare npm install is run, which installs dev deps.

It’s a scenario that multiple users expect to work, especially the ones that have been around since the beginning of npm.

I’m not concerned with package size on disk - only the size in the application. Files that aren’t consumed in the application don’t matter, and FUD-producing “phobia” sites don’t make it a valid concern.

@kazushisan
Copy link
Author

I understand what you meant now, I wasn't aware of use-cases running npm install under node_modules/. Thanks for clarifying.

@kazushisan kazushisan closed this Dec 13, 2024
@kazushisan kazushisan deleted the chore/specify-files branch December 13, 2024 03:39
@shadowspawn
Copy link
Collaborator

For interest, I thought we might have accidentally started including tests and looked into it in #23

Turned out Minimist had included tests in the package for years, probably always, and some people think including the development files is a good thing too. I don't do it myself in other packages I maintain, but also don't consider it a fault when done on purpose, with the benefits/use-cases @ljharb outlined.

@kazushisan
Copy link
Author

Thanks! My apologies for not noticing the issue...

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.

3 participants