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

fix: don't publish unneeded files #31

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@ffflorian
Copy link

commented Jul 4, 2019

This PR will prevent unneeded files from being published to npm.

Before:

package
├── auto.js
├── CHANGELOG.md
├── implementation.js
├── index.js
├── LICENSE
├── package.json
├── polyfill.js
├── README.md
├── shim.js
└── test
    ├── index.js
    ├── shimmed.js
    └── tests.js

1 directory, 12 files

After:

package
├── auto.js
├── CHANGELOG.md
├── implementation.js
├── index.js
├── LICENSE
├── package.json
├── polyfill.js
├── README.md
└── shim.js

0 directories, 9 files
@ljharb
Copy link
Owner

left a comment

Almost all of these files are needed; npm install foo && npm explore foo && npm install && npm test should always work.

@@ -49,6 +49,9 @@
"eslint": "^5.12.1",
"tape": "^4.9.2"
},
"files": [

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 4, 2019

Owner

The files field is very dangerous and should never be used; only npmignore should be used to restrict what is published.

This comment has been minimized.

Copy link
@ffflorian

ffflorian Jul 4, 2019

Author

@ljharb care to explain why it is so dangerous? There are some bad stories on using .npmignore and even npm suggests to use files:

There’s an even better way of controlling exactly which files are published with your package: whitelisting with the files array. Only 57,000 packages use this method of controlling what goes into them, probably because it requires you to take inventory of your package. It’s by far the safest way to do it, though.

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 4, 2019

Owner

npm is wrong. The “bad stories” of npmignore is that one article of someone who ignored best practices and stored credentials within the filesystem of the repo - there’s tons of issues all over github of when someone has forgotten to include a file on “files” and broken their consumers. I’m quite sure.

@ffflorian

This comment has been minimized.

Copy link
Author

commented Jul 4, 2019

@ljharb What do you mean by "Almost all of these files are needed", why are test files needed in a published project?

@ljharb

This comment has been minimized.

Copy link
Owner

commented Jul 4, 2019

Because people should be able to run the tests without having to go to the github repo, eg offline.

@ffflorian

This comment has been minimized.

Copy link
Author

commented Jul 4, 2019

@ljharb I see that we have different opinions on which files should be published. Therefore I am closing this PR. Have a good day! ☀️

@ffflorian ffflorian closed this Jul 4, 2019

@ffflorian ffflorian deleted the ffflorian:fix/files branch Jul 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.