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] npm publish ignores files exclusion glob #2441

Closed
dnalborczyk opened this issue Jan 4, 2021 · 10 comments · Fixed by dherault/serverless-offline#1542
Closed

[BUG] npm publish ignores files exclusion glob #2441

dnalborczyk opened this issue Jan 4, 2021 · 10 comments · Fixed by dherault/serverless-offline#1542
Assignees
Labels
Bug thing that needs fixing Priority 1 high priority issue Release 9.x work is associated with a specific npm 9 release semver:major backwards-incompatible breaking changes

Comments

@dnalborczyk
Copy link

dnalborczyk commented Jan 4, 2021

Current Behavior:

npm publish --dry-run includes __tests__ and __mocks__

package.json:

  "files": [
    "dist/**",
    "!__mocks__",
    "!__tests__",
    "package.json",
    "README.md"
  ]

Expected Behavior:

should exclude __tests__ and __mocks__

works with npm v6.14.9.

Steps To Reproduce:

see above

Environment:

  • OS: MacOs Catalina v10.15.7
  • Node: 15.5.0
  • npm: 7.3.0
@dnalborczyk dnalborczyk added Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release labels Jan 4, 2021
@gustavohenke
Copy link

gustavohenke commented Jan 4, 2021

👋 I'm also seeing this, after upgrading to v7 and publishing express-validator, files that were previously ignored started affecting consumers.

express-validator/express-validator#973
https://github.com/express-validator/express-validator/blob/297764945007b700371ff78b4a4add378aede698/package.json#L19-L27

Note: the lines above have been unchanged for about 2 years.

@shadowspawn
Copy link
Contributor

The documentation for files does not mention support for negating an option. I expect a work-around is to simply remove the negated lines. Possibly an intended change in behaviour.

https://docs.npmjs.com/cli/v6/configuring-npm/package-json#files

@dnalborczyk
Copy link
Author

dnalborczyk commented Jan 5, 2021

@shadowspawn

The documentation for files does not mention support for negating an option.

from the docs you linked: or glob pattern (*, **/*, and such)

I expect a work-around is to simply remove the negated lines.

that's not a workaround. the files are still being published. the whole point of negating is to exclude files from publishing.

Possibly an intended change in behaviour.

Possibly, although unlikely. this has worked for years, I assume since files was introduced. I think it's just a bug being introduced in v7, which is, if I remember correctly, a bigger rewrite.

@shadowspawn
Copy link
Contributor

shadowspawn commented Jan 5, 2021

I expect a work-around is to simply remove the negated lines.

that's not a workaround.

Oh, are the problem files in __tests__ somewhere under dist/**? On first read I assumed the patterns were not overlapping, and the exclusion glob was behaving as an inclusion.

Reproduced issue with node v15.5.1 and npm 7.3.0 on macOS v15.1.

Possibly an intended change in behaviour

Possibly, although unlikely.

I agree! I made a typo. I meant "unintended change".

@gustavohenke
Copy link

from the docs you linked: or glob pattern (*, **/*, and such)

Additionally: the docs mention "File patterns follow a similar syntax to .gitignore", so I expected anything that works there to work in the files array.

@shadowspawn
Copy link
Contributor

Thanks @gustavohenke and @dnalborczyk for pointing out the relevant information in the documentation I linked. My apologies, I was wrong with suggestion that files might use simpler syntax.

@wraithgar
Copy link
Member

Confirmed this is a changing behavior from 6 to 7

~/D/n/foo $ npx npm@6 publish --dry-run
Need to install the following packages:
  npm@6
Ok to proceed? (y) y
npm notice 
npm notice 📦  foo@1.0.0
npm notice === Tarball Contents === 
npm notice 6B   dist/index.js
npm notice 401B package.json 
npm notice 3B   README.md    
npm notice === Tarball Details === 
npm notice name:          foo                                     
npm notice version:       1.0.0                                   
npm notice package size:  406 B                                   
npm notice unpacked size: 410 B                                   
npm notice shasum:        91fb8aa3b4d3d7891ab1a8340d209ffb4c39d642
npm notice integrity:     sha512-g76XXMn/nll+6[...]HXtFYalqhCB9w==
npm notice total files:   3                                       
npm notice 
+ foo@1.0.0
~/D/n/foo $ npx npm@7 publish --dry-run
Need to install the following packages:
  npm@7
Ok to proceed? (y) y
npm notice 
npm notice 📦  foo@1.0.0
npm notice === Tarball Contents === 
npm notice 3B   README.md              
npm notice 0B   dist/__mocks__/foo.json
npm notice 6B   dist/__tests__/index.js
npm notice 6B   dist/index.js          
npm notice 401B package.json           
npm notice === Tarball Details === 
npm notice name:          foo                                     
npm notice version:       1.0.0                                   
npm notice filename:      foo-1.0.0.tgz                           
npm notice package size:  467 B                                   
npm notice unpacked size: 416 B                                   
npm notice shasum:        25ea3021a75979fd9b30b3abd8c76e781deb7350
npm notice integrity:     sha512-GNQTXwW8Z3+sz[...]aMXRJSG78irJA==
npm notice total files:   5                                       
npm notice 
+ foo@1.0.0

@hisham
Copy link

hisham commented Jul 13, 2021

+1.

This was also mentioned as fixed in #2009, but clearly was not.

@darcyclarke darcyclarke added the Priority 1 high priority issue label Mar 28, 2022
@nlf nlf self-assigned this Mar 28, 2022
@darcyclarke darcyclarke added semver:major backwards-incompatible breaking changes Release 9.x work is associated with a specific npm 9 release and removed Release 7.x work is associated with a specific npm 7 release labels Apr 20, 2022
@nlf
Copy link
Contributor

nlf commented Apr 20, 2022

just a quick update for now:

i've refactored npm-packlist to behave more like it did in npm@6 while still keeping most of the benefits of the state it's in today. unfortunately, we've decided that this change is a breaking change. yes, it restores the behavior of npm@6 but because the current behavior has been around for 2 semver-major ranges changing it now is another breakage.

due to this, we plan to land the refactor that fixes this issue as part of the first release of npm@9 which should hopefully not be too much longer.

for reference, here's the pull request with the refactor: npm/npm-packlist#88

@rauschma
Copy link

rauschma commented Mar 9, 2024

In case you’re interested, this is part of my package.json:

"files": [
  "package.json",
  "README.md",
  "LICENSE",
  "dist/**/*.js",
  "dist/**/*.js.map",
  "dist/**/*.d.ts",
  "dist/**/*.d.ts.map",
  "src/**/*.ts",
  "!dist/**/*_test.js",
  "!dist/**/*_test.js.map",
  "!dist/**/*_test.d.ts",
  "!dist/**/*_test.d.ts.map",
  "!src/**/*_test.ts"
],

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Priority 1 high priority issue Release 9.x work is associated with a specific npm 9 release semver:major backwards-incompatible breaking changes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants