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

Is it not possible to test .npmignore using this library? #61

Closed
octogonz opened this issue May 29, 2020 · 6 comments
Closed

Is it not possible to test .npmignore using this library? #61

octogonz opened this issue May 29, 2020 · 6 comments

Comments

@octogonz
Copy link

I have an .npmignore file like this:

# Ignore everything
**
# Bring back just the "dist" folder
!/dist/**

When I do npm pack, NPM selects everything under the "dist" folder.

Whereas node-ignore does not seem to reflect this behavior:

let ig = require('ignore')();
ig.add("**");
ig.add("!/dist/**");

# --> prints: true
ig.ignores("dist/bundle.js")
@octogonz
Copy link
Author

I noticed that the docs say:

  • Exactly according to gitignore man page, fixes some known matching issues of fstream-ignore, such as:
    • '/*.js' should only match 'a.js', but not 'abc/a.js'.
    • '**/foo' should match 'foo' anywhere.
    • Prevent re-including a file if a parent directory of that file is excluded.
    • Handle trailing whitespaces:
      • 'a '(one space) should not match 'a '(two spaces).
      • 'a \ ' matches 'a '
    • All test cases are verified with the result of git check-ignore.

That's great and everything about better accuracy for .gitignore files. Perhaps NPM deviates somewhat from this standard, by using fstream-ignore. But is there an option to simulate NPM's behavior?

If not, please document more clearly that this library is useless with .npmignore files, because I just wasted an hour trying to debug why there are weird inaccuracies in the selection results. :-)

@kaelzhang
Copy link
Owner

kaelzhang commented May 29, 2020

.npmignore does NOT follow the gitignore rules.

fstream-ignore depends on minimatch which basicly follows fnmatch(3) and its specific rules.

So the answer is NO (, it is not possible).

PS: I suggest to always use field package.json.files instead of .npmignore, because we only know which files should be included in the npm tarball

@octogonz
Copy link
Author

Would you consider implementing support for NPM rules as an option? Maybe it is not a difficult enhancement to make.

Alternatively, perhaps the README.md could have a disclaimer making this limitation more clear?

@octogonz
Copy link
Author

PS: I suggest to always use field package.json.files instead of .npmignore, because we only know which files should be included in the npm tarball

This problem is easily solved by inverting the ignore expressions. Our files generally look like this:

# Ignore everything by default
**

# Use negative patterns to bring back the specific things we want to publish
!/bin/**
!/lib/**
!/dist/**

This same solution also works for other files like .prettierignore and .eslintignore, since they were also mistakenly designed as "ignore" lists instead of "include" lists.

The package.json files setting is NOT a good approach. It has a couple downsides:

  • Adding project settings to package.json makes that file larger, which hurts installation performance. The NPM registry REST protocol inexplicably transmits the full package.json contents for every historical version of a package. During installation, this HTTP GET frequently has a larger payload than the actual tarball being installed.
  • You cannot put code comments in the JSON file. Code comments are a pretty important requirement for a professional scenario such as package publishing.

@kaelzhang
Copy link
Owner

kaelzhang commented May 29, 2020

Would you consider implementing support for NPM rules as an option? Maybe it is not a difficult enhancement to make.

If you read the .gitignore spec carefully, you will figure out that npmignore is completely a different story. For npmignore, you could use minimatch (for just pattern parsing) or fstream-ignore (with file system)

Alternatively, perhaps the README.md could have a disclaimer making this limitation more clear?

Yes

@octogonz
Copy link
Author

octogonz commented May 30, 2020

👍 Thanks!

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 a pull request may close this issue.

2 participants