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

Ensure no extraneous files get published #103

Closed
jfmengels opened this issue Nov 18, 2016 · 25 comments · Fixed by #456
Closed

Ensure no extraneous files get published #103

jfmengels opened this issue Nov 18, 2016 · 25 comments · Fixed by #456
Labels
enhancement 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted

Comments

@jfmengels
Copy link

jfmengels commented Nov 18, 2016

Issuehunt badges

I think it will be nice if, before publishing, np checked whether the package has either:

  • set a files field in package.json
  • a .npmignore file

This would help publishers make cleaner packages and reduce the bandwidth used when installing the resulting package.

I would actually advocate towards making sure there is a files field, as it's better IMO than using .npmignore, but that's just my opinion. A well designed .npmignore has the same result.


IssueHunt Summary

bunysae bunysae has been rewarded.

Backers (Total: $60.00)

Submitted pull Requests


Tips

@sindresorhus
Copy link
Owner

👍 👍 I can't believe I didn't think of that. I'm like the biggest proponent of the files property.

How about failing if there's neither and warn about recommending files if there's a .npmignore?

@jfmengels
Copy link
Author

I had the same thought :D

I'd be fine with that. I think you should you be able to force through the error with an option though.

@sindresorhus
Copy link
Owner

I think you should you be able to force through the error with an option though.

Why? There's absolutely no reason to not use either files or .npmignore.

@SamVerschueren
Copy link
Collaborator

I agree with Sindre. It gives me the creeps to see test files being part of a shipped dependency...

@jfmengels
Copy link
Author

I'm thinking of the case when you started a new repo and want/need to publish it quickly for some reason and you don't want to mess up the package by quickly adding a files property without checking it's right. Maybe you don't have the time to check it because it's very urgent and you'd need to make a PR to add the files field and get it approved by your team, etc...

Ok, maybe it's a bit farfetched, I agree. But other than this case, I don't see a good reason no.

I thought about letting this pass when the package is so simple that it would not publish anything extra anyway, but I'm not sure it's worth going through the trouble of implementing it for what seems like a very edgy case.

you want to publish your repo for the

@SamVerschueren
Copy link
Collaborator

I would suggest adding this functionality without an option. If someone comes up with a valid use case, we can always reconsider.

@jfmengels
Copy link
Author

Yeah, let's do that. Anyway, if this case comes up, they could still use the original npm publish

@voxpelli
Copy link
Contributor

The only thing that makes me a bit anxious is that of the two solutions, files and npmignore, both require that all team members remember to either add new files that should be ignored to npmignore (failure could in worst case result in uploaded secrets) or to add new files to files (failure to do so could in worst case result in broken code).

Currently I have opted to push for np internally at work as it's the most fool proof way of publishing new versions of npm packages – with the running of tests and everything.

Will be hard though to test whether files includes everything it should or whether npmignore ignores everything that it should. Enforcing that ones always use one or another in a team that isn't used to working with them sounds like a recipe for broken packages.

I'm 👍 on adding a warning though, especially as a first step, to nudge people in this direction. If one could then somehow add a safety net to catch missing additions to either files or npmignore then I would have no problem with enforcing it either, but unfortunately I don't think such a safety net can't be constructed.

@sindresorhus
Copy link
Owner

either add new files that should be ignored to npmignore (failure could in worst case result in uploaded secrets)

How is this worse than not using .npmignore though?

Will be hard though to test whether files includes everything it should

See: #82

@voxpelli
Copy link
Contributor

@sindresorhus It's worse as .npmignore overrides .gitignore, which hopefully everyone knows how to use, so anyone who expects a file ignored in .gitignore to be excluded from publishing (as is the default of npm) will accidentally be publishing files it shouldn't

@fregante
Copy link
Collaborator

Related: perhaps there can be a size check too/instead. Shipping a hundred 1KB files (like tests, usually) is not as bad as a 5MB package. To put things into perspective, all babel's tests take 750KB of disk space (although they are 4000 files)

@fregante
Copy link
Collaborator

fregante commented Nov 18, 2016

@voxpelli gitignore might be ignored at some point:

Stop using .gitignore as a fallback for .npmignore npm/npm#12331

@voxpelli
Copy link
Contributor

@bfred-it Totally and that would solve my anxiety here that enforcing a mechanism like this on people not expecting a mechanism like this will result in them doing things that goes against their intentions.

But we're not there now so until we are I will be restrictive in adding either files or .npmignore to projects as the risk of someone forgetting to update them is too large and I would like it if np allowed me to keep it that way as all of the rest of np:s features helps people in delivering on their intentions rather than enforces things that might have them do the opposite.

So I'm 👍 on any nudging to raise awareness about this issue in the meanwhile though, but 👎 on any enforcing until a change like the one you mention has been implemented.

@SamVerschueren
Copy link
Collaborator

If we would enforce it, you can easily just add an empty .npmignore file, no? I'm not very enthusiastic about adding complex checks on file size etc. It will fail at some point. So if you want to work around this one, I would just suggest adding an empty .npmignore. At least, it makes you aware of the issues about not adding a files property or a .npmignore file.

@voxpelli
Copy link
Contributor

@SamVerschueren Simply adding an empty .npmignore makes npm ignore the .gitignore and thus go against the expectations of all of those who are used to npm respecting the contents of .gitignore, so no, that would not help

@jfmengels
Copy link
Author

@voxpelli That's a fair point. But instead of creating an empty .npmignore, you could copy the contents of .gitignore. Knowing that option, would that rid you of your anxiety? Would having an option to override this default behavior help otherwise?

@itaisteinherz
Copy link
Collaborator

Is there a verdict then?
It seems to me like for now, warning users about using .npmignore / not having it at all, and recommending to use the files property (because of the reasons mentioned above) would be a good way to get this landed in np. We can always change the warning to be an error.

@IssueHuntBot
Copy link

@IssueHunt has funded $60.00 to this issue.


@sindresorhus
Copy link
Owner

sindresorhus commented Apr 18, 2019

@itaisteinherz Yes, let's start with a warning, and we can consider making it an error in the future if the warning works out well. I would show the warning at startup before the publish process begins.

I wonder if we could check with Git if any files were added since the last release, and if so, we could check if they match any patterns in files or .npmignore and if not, we could show them so users would be aware. It could help catch mistakes for both forgetting to add a file to the files field and forgetting to add a file to .npmignore. Thoughts?

@itaisteinherz
Copy link
Collaborator

itaisteinherz commented Jun 3, 2019

I would show the warning at startup before the publish process begins.

@sindresorhus Even before the ui.js, or after that but before publishing begins?

@sindresorhus
Copy link
Owner

@itaisteinherz I would show it before/above Publish a new version of github-username (current: 5.0.1).

@sindresorhus
Copy link
Owner

The files field and .npmignore check was added in #411.

We still need to do:

I wonder if we could check with Git if any files were added since the last release, and if so, we could check if they match any patterns in files or .npmignore and if not, we could show them so users would be aware. It could help catch mistakes for both forgetting to add a file to the files field and forgetting to add a file to .npmignore.

@bunysae
Copy link
Contributor

bunysae commented Sep 9, 2019

I read the discussion and i wanted to started working on this.
List up the files added since the last release, i use often for code-reviews.
If the last release has a git-tag, we could use the git diff command to find out the files, which were added since the last release.

@cspotcode
Copy link

I while ago I created a CLI tool called comprehensive-npmignore to do this.
https://www.npmjs.com/package/comprehensive-npmignore

It asserts that you have explicitly declared all files as either included or excluded.

It does not do anything with git, which makes sense in case you have non-versioned build artifacts that you must remember to declare.

Anyway, I see that #456 is almost done already, but I'll leave this here anyway since the implementation is a bit different. Maybe future versions of np will do something similar.

@issuehunt-oss
Copy link

issuehunt-oss bot commented Oct 29, 2020

@sindresorhus has rewarded $54.00 to @bunysae. See it on IssueHunt

  • 💰 Total deposit: $60.00
  • 🎉 Repository reward(0%): $0.00
  • 🔧 Service fee(10%): $6.00

@issuehunt-oss issuehunt-oss bot added 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt and removed 💵 Funded on Issuehunt This issue has been funded on Issuehunt labels Oct 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants