Please give an error when files don't exist

[petkaantonov]

I have defined a files field in my package.json and I don't think npm should publish a broken version when any of those files are missing.

[othiym23]

At the very least it should warn, but yeah, it would be best if it printed an error and stopped when a member of files is missing. What's the behavior you're seeing right now? Silently ignoring missing files?

[petkaantonov]

I forgot if I saw any warning but it definitely published a broken version

[smikes]

Silently ignores missing files. Added test set in PR #6618 , with repro steps.

Tracked this through several modules, appears to be a problem in fstream-npm or node-tar.. possibly node-tar is not throwing/reporting an error when an expected file is missing.

Looks like fstream-npm in readRules creates an inclusion rule as a sort of reverse ignore rule: ignore everything ('*') except for not-the-things-in-files. Apparently this fails for files that are missing. Will check in fstream-ignore.

[rlidwka]

Those aren't files, those are patterns as far as I remember. If you specify files: ["*.js"], what file it should warn about?

[smikes]

Confirmed that the problem occurs when fstream-npm (and underlyingly fstream-ignore) receive an ignore list which specifies that file should not be ignored, when file is actually missing.

e.g., ignore: ["*", "!file1", "!file2"] if the directory contains only file1 and file3 will produce ["file1"]

See npm/fstream-ignore#9 for a test that shows this. (This PR is for illustration only)

[smikes]

@rlidwka When they are patterns, this logic cannot be applied. But when they contain no wildcard characters and no file matches an entry in files, an error can safely be emitted. The package author can either remove the entry from files or add a matching file.

[smikes]

Thinking about this a bit more; @rlidwka 's point is that the files[] array is defined to be a list of patterns. There is no general-case way to decide whether an entry in files should be a file or a pattern, and the chain of dependencies (lib/pack.js -> fstream-npm -> fstream-ignore -> node-tar) does not offer a natural place to place a check on whether particular files have made it into the tarball.

However, it is possible to build the tarball before publishing by running npm pack. So there could be a script that runs npm pack, checks that the files named in files are present, and if so, deletes the tarball and runs npm publish. @petkaantonov would that meet your requirements to prevent publishing a broken version of the package?

[petkaantonov]

@smikes sure

[othiym23]

I think it would be useful to warn or error if any of the individual entries in files don't match. I agree that's more complicated than an outboard script like @smikes describes, because changes would probably have to be made in a few modules to get it working properly, but it would make for a more useful feature / less footgunning.

[smikes]

@othiym23 Sounds plausible.

For now, I will write the external code that inspects a pack file and its package.json and checks for entries of files, if it is not a glob (containing !*?) then the pack file contains a file that looks like that.

Then if that turns out to be a good thing to include in npm, I can deed the snippet or package over to the npm organization. At present I am a bit overwhelmed by the number of places it would be possible to insert that check in npm-pack and its related/dependent modules.

[isaacs]

The problem is not in fstream-ignore, and since (as @rlidwka points out) the entries in "files" array are patterns rather than filenames, it'd be impossible to know if any are missing.

[smikes]

The problem is not in fstream-ignore

Agreed. What I was trying to demonstrate (to my own satisfaction) was that the file list is arising much deeper than the call to pack or _pack

impossible to know if any are missing.

Impossible in the general case. Very possible in the way that @petkaantonov is (ab)using the files list of patterns.

This issue is actually a feature request: A. detect whether critical files are missing from the pack (and if so, B. don't let me shoot myself in the foot by publishing) . I propose to write an external utility to answer question A. Then I will test it against a sample of published modules to estimate the risk in turning this into a on-by-default feature.

If nothing else I'll learn something about tar files.

[petkaantonov]

The problem is not in fstream-ignore, and since (as @rlidwka points out) the entries in "files" array are patterns rather than filenames, it'd be impossible to know if any are missing.

It's possible to know if no files matched a pattern, and I am not using any pattern anyway but directory and direct file references.

[smikes]

I expected files to behave like a list of possibly globbed arguments on a shell command line. But in fact they work as exclusions to a global ignore rule.

The difference is that when I execute ls foo* I expect to see an error if nothing matches the pattern (ls: foo*: No such file or directory). When I use files: ["foo*"] though, I am implicitly asking for glob("*", "!foo*") -- and unlike the simple shell glob, empty set is a legal return value.

[isaacs]

"scripts":{"prepublish":"ls foo*"} would do the trick in the short term.

[smikes]

Nice.

This will read the "files" property from package.json

"prepublish": "ls `node -e 'console.log((require(\"./package.json\").files||[]).join(\" \"))'`>/dev/null",

I tested with missing files, valid files in files, valid globs in files and invalid globs in files. Note that this will not support **/*.js syntax unless the shell supports it.

[isaacs]

By the way, I'm not saying I'm necessarily opposed to adding the "warn on unmatched globs in files array" feature. But it is tricky to get right, so testing out the semantics with a workaround is probably a good idea.

What if you have "files":["*.js","*.foo"], and there are no *.foo files. Should that be a problem as well? (It will be for ls, so I guess yeah, probably?)

This would basically mean that we need to add another step in the package, or at least, keep track of what the needed files matches are, so that we can warn if any of them are not matched.

[othiym23]

This has been moved to the npm roadmap, which we're using instead of the confusing next-* labels now.

[zkat]

Hey @petkaantonov!

I had originally poked at a version of this change with #12191 (which was connected to #8791) during a Files and Ignores push we had earlier this year, but it got waylaid because it required some bigger changes to fstream with the way I was doing it, and other things ended up taking priority.

That said! The team talked it over today, and we agreed that this might be a good thing to have (done a bit differently than the original one -- most notably making it an error, as you requested here). This would make it a breaking change, but we don’t have a schedule for when we’re going to implement it right now.

Most likely, what we’ll end up doing is add something at the end of pack that checks the final tarball’s file list and errors if any single entry/glob in files fails to match at least once. I assume that would fulfill your request here :)

So yeah! We’re gonna leave this open, since we do intend to get to it ourselves some time, but I can’t tell you exactly when it’ll land. Thanks for your time!

[npm-robot]

We're closing this issue as it has gone thirty days without activity. In our experience if an issue has gone thirty days without any activity then it's unlikely to be addressed. In the case of bug reports, often the underlying issue will be addressed but finding related issues is quite difficult and often incomplete.

If this was a bug report and it is still relevant then we encourage you to open it again as a new issue. If this was a feature request then you should feel free to open it again, or even better open a PR.

For more information about our new issue aging policies and why we've instituted them please see our blog post.