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

[Bug?]: .npmignore is not processed like gitignore; ** doesn't match files at the root, order not respected #5872

Open
1 task
jakebailey opened this issue Oct 26, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@jakebailey
Copy link

Self-service

  • I'd be willing to implement a fix

Describe the bug

.npmignore is theoretically supposed to have identical semantics to gitignore. However, in testing stuff out for DefinitelyTyped (so that packing works for any package manager people may run on the repo), I noticed that yarn's implementation doesn't match other package managers and git. https://git-scm.com/docs/gitignore#_pattern_format

First, **/*.d.ts should be able to match a .d.ts file at the root, but only matches d.ts files within other packages. ** is documented as allowing "no match at all" per the gitignore docs.

A leading "**" followed by a slash means match in all directories. For example, "**/foo" matches file or directory "foo" anywhere, the same as pattern "foo". "**/foo/bar" matches file or directory "bar" anywhere that is directly under directory "foo".

Secondly, the order isn't respected; if I exclude something, it should be possible to re-include things (with some exceptions). See https://stackoverflow.com/a/24678813 and the docs:

An optional prefix "!" which negates the pattern; any matching file excluded by a previous pattern will become included again. It is not possible to re-include a file if a parent directory of that file is excluded.

On DefinitelyTyped, we want to have ignored files something like this:

*
!**/*.d.ts
/v1/

Which is to say for a package, include only dts files (there is also cts/mts/etc but I am omitting them for brevity), but not their child version packages. The above works for npm and pnpm, but not yarn at any version.

Skimming, I think the latter case comes down to the include/exclude just being two lists, but I'm no expert.

To reproduce

Here is a repo that shows things: https://github.com/jakebailey/yarn-npmignore-ordering

Environment

System:
    OS: Linux 5.15 Ubuntu 22.04.3 LTS 22.04.3 LTS (Jammy Jellyfish)
    CPU: (20) x64 Intel(R) Core(TM) i9-10900K CPU @ 3.70GHz
  Binaries:
    Node: 20.8.1 - /tmp/xfs-cd293fcb/node
    Yarn: 4.0.0 - /tmp/xfs-cd293fcb/yarn

Additional context

No response

@arcanis
Copy link
Member

arcanis commented Oct 26, 2023

First, **/*.d.ts should be able to match a .d.ts file at the root, but only matches .d.ts files within other packages.

I'll check that - I think Git is sometimes weird with ** being either "zero or more" or sometimes "one or more" ... perhaps we implemented the wrong one? We probably won't be able to change it until the next major though (unfortunate given that we just released 4.0 ☹️).

.npmignore is theoretically supposed to have identical semantics to gitignore

The above works for npm and pnpm, but not yarn at any version.

Yes, that's a bug in npm (and pnpm, since it shells out to npm for packing); they don't actually behave like a gitignore:

cd $(mktemp -d)
git init && npm init -y
mkdir -p a/b/c
touch foo.ts a/b/c/foo.ts
touch foo.d.ts a/b/c/foo.d.ts
echo '*' >> .gitignore
echo '!**/*.d.ts' >> .gitignore

git status (it doesn't see a/b/c/foo.d.ts, because it never goes inside a)

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	foo.d.ts

npm pack --dry-run (it sees a/b/c/foo.d.ts, but it shouldn't)

npm notice === Tarball Contents ===
npm notice 0B   a/b/c/foo.d.ts
npm notice 0B   foo.d.ts

We try to follow the npm behaviour when possible, but since it doesn't even follow its own documentation it's quite difficult to be sure that there's 100% parity 😅

@arcanis
Copy link
Member

arcanis commented Oct 26, 2023

For now I added some tests and a couple of TODOs in #5873.

arcanis added a commit that referenced this issue Oct 26, 2023
**What's the problem this PR addresses?**

#5872 made me notice that we have few tests covering how the `*` / `**`
patterns are interpreted.

**How did you fix it?**

This diff fixes that by adding a couple of tests, along with comments on
the few places where we seem to diverge from npm. It doesn't try to
change the pack list generation, just to increase its coverage and make
it easier to add more tests in the future.

**Checklist**
<!--- Don't worry if you miss something, chores are automatically
tested. -->
<!--- This checklist exists to help you remember doing the chores when
you submit a PR. -->
<!--- Put an `x` in all the boxes that apply. -->
- [x] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).

<!-- See
https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released
for more details. -->
<!-- Check with `yarn version check` and fix with `yarn version check
-i` -->
- [x] I have set the packages that need to be released for my changes to
be effective.

<!-- The "Testing chores" workflow validates that your PR follows our
guidelines. -->
<!-- If it doesn't pass, click on it to see details as to what your PR
might be missing. -->
- [x] I will check that all automated PR checks pass before the PR gets
reviewed.
@jakebailey
Copy link
Author

I'll check that - I think Git is sometimes weird with ** being either "zero or more" or sometimes "one or more" ... perhaps we implemented the wrong one? We probably won't be able to change it until the next major though (unfortunate given that we just released 4.0 ☹️).

Apologies, I know it's bad timing, we just so happened to do the big DT migration and are working on the small stuff.

Yes, that's a bug in npm (and pnpm, since it shells out to npm for packing); they don't actually behave like a gitignore:

Hm, in my testing using an npmignore similar to my repro, all package managers behaved as expected except yarn on this front. I'll have to retest and make sure that I'm actually observing all of the files I'm intending to include in all of these cases.

FWIW this isn't blocking or anything; the DT publisher copies files of its choosing out of the DT checkout into a tempdir for publishing, so we can always publish exactly what we want. But, a soft goal with the new "we actually have package.json" layout is that someone could feasibly drive-by npm pack a folder and get something that reasonably emulates the real thing as that's actually pretty useful for tooling like arethetypeswrong, hence trying to get some form of configuration that works.

@jakebailey
Copy link
Author

Ah, I see what you mean in that example. I guess this is a case where the "directories cannot be added back" is not being followed by npm, since * excludes it? Seems confusing because the examples from stackoverflow imply that it's okay to exclude everything then include everything back again... Maybe I'm missing something. It'd be unfortunate if what I'm asking for is in fact not covered by what gitignore does, but some other specification, ugh.

The best real-world example I have is https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/ember-data with:

*
!**/*.d.ts
!**/*.d.cts
!**/*.d.mts
!**/*.d.*.ts
/v2/
/v3/

Giving me tar contents of:

package/adapter.d.ts
package/adapters/errors.d.ts
package/adapters/json-api.d.ts
package/adapters/rest.d.ts
package/attr.d.ts
package/index.d.ts
package/model.d.ts
package/package.json
package/relationships.d.ts
package/serializer.d.ts
package/serializers/embedded-records-mixin.d.ts
package/serializers/json-api.d.ts
package/serializers/json.d.ts
package/serializers/rest.d.ts
package/store.d.ts
package/transform.d.ts
package/transforms/boolean.d.ts
package/transforms/date.d.ts
package/transforms/number.d.ts
package/transforms/string.d.ts
package/transforms/transform.d.ts
package/types/registries/adapter.d.ts
package/types/registries/model.d.ts
package/types/registries/serializer.d.ts
package/types/registries/transform.d.ts

npm 6-10 and pnpm agree, but yarn 1-4 don't exclude the dirs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants