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.

Sign up for GitHub

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

Jump to bottom

Support excluding specific file patterns in nested directories #29

Closed
wants to merge 1 commit into from
Closed

Support excluding specific file patterns in nested directories #29

wants to merge 1 commit into from

Conversation

Shingyx
Copy link

@Shingyx Shingyx commented Apr 11, 2019

For a file pattern such as dist/**/!(*.test.js), previously it would add an entry of !dist/**/!(*.test.js)/** for the ignore-walker, which results in paths such as dist/lib/x.test.js getting included in the pack. The fix is to avoid appending the /** if the file ends in **/!(pattern).

The CLI output below on a temporary project shows that the current behaviour does not match the glob spec when it comes to excluding file patterns. 

shingy@shingy-vm:~/source/junk$ find . | grep -v node_modules
.
./dist
./dist/index.js
./dist/index.d.ts
./dist/index.test.d.ts
./dist/lib
./dist/lib/utilities.test.d.ts
./dist/lib/utilities.d.ts
./dist/lib/utilities.test.js
./dist/lib/utilities.js
./dist/index.test.js
./src
./src/index.ts
./src/lib
./src/lib/utilities.ts
./src/lib/utilities.test.ts
./src/index.test.ts
./package.json
./yarn.lock
./tsconfig.json
shingy@shingy-vm:~/source/junk$ grep files package.json && npm pack
  "files": ["dist/**/!(*.test.*)"],
npm notice
npm notice 📦  junk@1.0.0
npm notice === Tarball Contents ===
npm notice 203B package.json
npm notice 0    dist/index.d.ts
npm notice 14B  dist/index.js
npm notice 0    dist/lib/utilities.d.ts
npm notice 14B  dist/lib/utilities.js
npm notice 0    dist/lib/utilities.test.d.ts
npm notice 14B  dist/lib/utilities.test.js
npm notice === Tarball Details ===
npm notice name:          junk
npm notice version:       1.0.0
npm notice filename:      junk-1.0.0.tgz
npm notice package size:  390 B
npm notice unpacked size: 245 B
npm notice shasum:        4d09c3f24bb4c12f9249d932662275a3a03d64f4
npm notice integrity:     sha512-XLn+lIxjY9Wn6[...]Kelvri0OiYskQ==
npm notice total files:   7
npm notice
junk-1.0.0.tgz
shingy@shingy-vm:~/source/junk$ node -p "require('glob').sync(require('./package.json').files[0], { nodir: true })"
[ 'dist/index.d.ts',
  'dist/index.js',
  'dist/lib/utilities.d.ts',
  'dist/lib/utilities.js' ]

Thanks!

@Shingyx
Support excluding specific file patterns in nested directories
93df8ff 
For a file pattern such as "dist/**/!(*.test.js)", previously it would
add an entry of "!dist/**/!(*.test.js)/**" for the ignore-walker, which
results in paths such as "dist/lib/x.test.js" getting included in the
pack. The fix is to avoid appending the "/**" if the file ends in
"**/!(pattern)".
@Shingyx
Copy link
Author

Shingyx commented Apr 11, 2019

Looks like the build is only failing on Node 4 due to ES6 syntax in a node module.

Shingyx
Shingyx commented Apr 11, 2019
View reviewed changes
index.js
@@ -165,7 +165,10 @@ const npmWalker = Class => class Walker extends Class {

if (Array.isArray(pkg.files))
super.onReadIgnoreFile('package.json', '*\n' + pkg.files.map(
f => '!' + f + '\n!' + f.replace(/\/+$/, '') + '/**'
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be a better solution if we change this to always return '!' + f, and then update ignore-walker to handle directories when the /** is missing? Currently if I do this, the tests for the package-json-bin-x.js tests fail, e.g.

npm-packlist/test/package-json-bin-single.js

Lines 24 to 31 in 1a4c8bf

const json = {
name: 'test-package',
version: '1.6.2',
bin: '__bin',
files: [
'lib'
]
}

@Shingyx Shingyx mentioned this pull request May 28, 2019
Closed
@Shingyx
Copy link
Author

Shingyx commented May 28, 2019

I don't think what I've done is the right solution, so I converted this to the issue #31 instead.

@Shingyx Shingyx closed this May 28, 2019
isaacs added a commit that referenced this pull request Oct 9, 2019
@isaacs
Handle package.json 'files' more intuitively
f953a41 
Rather than converting the package.json `files` array to a set of
reverse-ignore rules, resolve the globs, and treat the resulting file
list as if it was the result of the fs.readdir call at the start.

We still generate a set of "ignore" rules based on the presence of
files, but only to handle the negated cases, so that situations like
this behave as a user will tend to expect:

    {
      "files": [
        "lib/",
        "!lib/foo.js"
      ]
    }

This imposes some subtle and potentially risky changes.  Not risky
because they're bad or unexpected -- quite the opposite -- but simply
risky because it's an area of npm where users tend to just throw bits
at the wall and then ignore once it's sort of working.

Since npm only reads ignore files and package.json files on package
preparation and not extraction, the risk is limited.  Some folks might
update their client, and then publish some packages that don't behave
quite as they intend, but feedback on npm.community and other venues has
shown that in all cases where the v1 behavior differed from v2, users
were surprised and frustrated.

Close #14
Fix #29
isaacs added a commit that referenced this pull request Oct 9, 2019
@isaacs
Handle package.json 'files' more intuitively
2140d50 
Rather than converting the package.json `files` array to a set of
reverse-ignore rules, resolve the globs, and treat the resulting file
list as if it was the result of the fs.readdir call at the start.

We still generate a set of "ignore" rules based on the presence of
files, but only to handle the negated cases, so that situations like
this behave as a user will tend to expect:

    {
      "files": [
        "lib/",
        "!lib/foo.js"
      ]
    }

This imposes some subtle and potentially risky changes.  Not risky
because they're bad or unexpected -- quite the opposite -- but simply
risky because it's an area of npm where users tend to just throw bits
at the wall and then ignore once it's sort of working.

Since npm only reads ignore files and package.json files on package
preparation and not extraction, the risk is limited.  Some folks might
update their client, and then publish some packages that don't behave
quite as they intend, but feedback on npm.community and other venues has
shown that in all cases where the v1 behavior differed from v2, users
were surprised and frustrated.

Close #14
Fix #29
isaacs added a commit that referenced this pull request Oct 9, 2019
@isaacs
Handle package.json 'files' more intuitively
fe35a62 
Rather than converting the package.json `files` array to a set of
reverse-ignore rules, resolve the globs, and treat the resulting file
list as if it was the result of the fs.readdir call at the start.

We still generate a set of "ignore" rules based on the presence of
files, but only to handle the negated cases, so that situations like
this behave as a user will tend to expect:

    {
      "files": [
        "lib/",
        "!lib/foo.js"
      ]
    }

This imposes some subtle and potentially risky changes.  Not risky
because they're bad or unexpected -- quite the opposite -- but simply
risky because it's an area of npm where users tend to just throw bits
at the wall and then ignore once it's sort of working.

Since npm only reads ignore files and package.json files on package
preparation and not extraction, the risk is limited.  Some folks might
update their client, and then publish some packages that don't behave
quite as they intend, but feedback on npm.community and other venues has
shown that in all cases where the v1 behavior differed from v2, users
were surprised and frustrated.

Close #14
Fix #29
isaacs added a commit that referenced this pull request Oct 9, 2019
@isaacs
Handle package.json 'files' more intuitively
1492fb6 
Rather than converting the package.json `files` array to a set of
reverse-ignore rules, resolve the globs, and treat the resulting file
list as if it was the result of the fs.readdir call at the start.

We still generate a set of "ignore" rules based on the presence of
files, but only to handle the negated cases, so that situations like
this behave as a user will tend to expect:

    {
      "files": [
        "lib/",
        "!lib/foo.js"
      ]
    }

This imposes some subtle and potentially risky changes.  Not risky
because they're bad or unexpected -- quite the opposite -- but simply
risky because it's an area of npm where users tend to just throw bits
at the wall and then ignore once it's sort of working.

Since npm only reads ignore files and package.json files on package
preparation and not extraction, the risk is limited.  Some folks might
update their client, and then publish some packages that don't behave
quite as they intend, but feedback on npm.community and other venues has
shown that in all cases where the v1 behavior differed from v2, users
were surprised and frustrated.

Close #14
Fix #29
isaacs added a commit that referenced this pull request Oct 9, 2019
@isaacs
Handle package.json 'files' more intuitively
25225c9 
Rather than converting the package.json `files` array to a set of
reverse-ignore rules, resolve the globs, and treat the resulting file
list as if it was the result of the fs.readdir call at the start.

We still generate a set of "ignore" rules based on the presence of
files, but only to handle the negated cases, so that situations like
this behave as a user will tend to expect:

    {
      "files": [
        "lib/",
        "!lib/foo.js"
      ]
    }

This imposes some subtle and potentially risky changes.  Not risky
because they're bad or unexpected -- quite the opposite -- but simply
risky because it's an area of npm where users tend to just throw bits
at the wall and then ignore once it's sort of working.

Since npm only reads ignore files and package.json files on package
preparation and not extraction, the risk is limited.  Some folks might
update their client, and then publish some packages that don't behave
quite as they intend, but feedback on npm.community and other venues has
shown that in all cases where the v1 behavior differed from v2, users
were surprised and frustrated.

Close #14
Fix #29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@Shingyx