.DS_Store files show up after npm publish

cli
help-wanted
good-first-patch
priority:low
triaged

(Steven) #1

What I Wanted to Do

npm publish without .DS_Store files

What Happened Instead

npm publish worked but .DS_Store files are showing up in published artifact

Reproduction Steps

I can’t reproduce with this exact example but I assume you clone clipboardy and then npm publish

Details

Take a look at package.json in clipbaordy@1.2.3 and note the files key. Then take a look at the fallbacks folder and note the .DS_Store.

The npm docs explain that .DS_Store files are always ignored so I would not expect this to be there.

Related issue: https://github.com/sindresorhus/clipboardy/pull/41

Platform Info

$ npm --versions
{ clipboardy: '1.2.3',
  npm: '6.2.0',
  ares: '1.10.1-DEV',
  cldr: '32.0',
  http_parser: '2.8.0',
  icu: '60.1',
  modules: '57',
  napi: '3',
  nghttp2: '1.32.0',
  node: '8.11.3',
  openssl: '1.0.2o',
  tz: '2017c',
  unicode: '10.0',
  uv: '1.19.1',
  v8: '6.2.414.54',
  zlib: '1.2.11' }
$ node -p process.platform
win32

(Kat Marchán) #2

This is a bug in an earlier version of npm, I think, since that version of clipboardy was published with npm@5.6.0.

I can’t reproduce this with the latest npm by doing npm pack sindresorhus/clipboardy --dry-run, so I’m gonna consider this to be fixed. sindre can fix it by publishing with a newer npm version.


(Steven) #3

@zkat Oh I almost forgot about npm pack

I was able to reproduce this with the latest npm.

Here’s what I did.

git clone https://github.com/sindresorhus/clipboardy
cd clipboardy
touch fallbacks/.DS_Store
npm pack
tar -xzf clipboardy-1.2.3.tgz
cd package/fallbacks
# wtf I see .DS_Store

Here’s the npm --versions

{ clipboardy: '1.2.3',
  npm: '6.2.0',
  ares: '1.10.1-DEV',
  cldr: '32.0',
  http_parser: '2.8.0',
  icu: '60.1',
  modules: '57',
  napi: '3',
  nghttp2: '1.32.0',
  node: '8.11.3',
  openssl: '1.0.2o',
  tz: '2017c',
  unicode: '10.0',
  uv: '1.19.1',
  v8: '6.2.414.54',
  zlib: '1.2.11' }

(Kat Marchán) #4

ooooh. Right, I would’ve had to check out the repo first.

This looks like we have a bug in our midst. The relevant repo for this is https://github.com/npm/npm-packlist/blob/master/index.js#L33, but I’m not sure why that rule would be failing (it’s definitely there!).

Would you be interested in looking into it? It’s a fairly self-contained repo so it should be straightforward to at least test for. It looks like the default rules aren’t applying to nested files for some reason, which is super strange. I would check to see if other default rules also fail when nested. /cc @isaacs


(Steven) #5

I think I figured it out. See https://github.com/npm/npm-packlist/pull/14


(Isaac Z. Schlueter) #6

Ok, I tracked this down. (Will add to PR as well, for posterity.)

The issue here is that the files list in package.json explicitly includes the fallbacks folder. So, that means, files in that folder are included, because files in package.json takes precedence over the default rules.

If we re-apply the default rules at all levels, then that would certainly be a breaking change. It would mean that including a folder via files in package.json would not include all of its children. (Effectively, with this patch, it would be impossible to ever include a file at foo/.npmignore by doing "files": [ "foo" ] in package.json.)


(Kat Marchán) #7

I missed that fallbacks was in files, but yeah – it’s as isaac says. And, frankly, I don’t think the breaking change is worth the trouble.


(Steven) #8

Thanks, it appears you are right about files.

If we re-apply the default rules at all levels, then that would certainly be a breaking change.

I guess so…but it would mean that the code would match the documentation of package.json#files

Are you concerned someone would actually want to publish one of the ignored files in a child directory instead of the root directory?


(Kat Marchán) #9

If this is different than what npm@4 and earlier did, I would also consider this a bug and something we should fix, tbh.

Or even if this is different if you use npm@5.3, which is the version before we switched over to npm-packlist. I’d definitely consider this a bug and the “breaking change” issue would go away :slight_smile:


(Luigi Pinca) #10

Yes, in my opinion this is a regression that did go unnoticed for a very long time.


(Kat Marchán) #11

@isaacs @iarna I think we should take a patch to fix this, because I agree about this being a regression from our upgrade to npm-packlist. And, indeed, the last version before we switched over, npm@5.3.0, does, indeed, exclude fallbacks/.DS_Store.

p.s. The patch will have to be in either npm-packlist or ignore-walk. Not sure which one.


(Isaac Z. Schlueter) #12

Sounds good to me.

This should be fixed in ignorewalk. Precedent:

$ tree
.
├── bar/
│   ├── asdf
│   └── baz
├── .gitignore
└── baz

1 directory, 4 files

$ cat .gitignore
baz
!bar/

$ git add .

$ git status
On branch master

Initial commit

Changes to be committed:
  (use "git rm --cached <file>..." to unstage)

	new file:   .gitignore
	new file:   bar/asdf

I’d like to do a bit more research to make sure that the implementation properly addresses the questions I brought up in the PR comments.


(Isaac Z. Schlueter) #13

Er, actually, that’s not quite a fair comparison. Definitely needs more research.

Anyway, the point is that it ““should”” work ““the same as”” git and .gitignore rules, but git doesn’t really have a corollary to package.json#files, so… idk. It’s stickier than it looks at first glance.

I do not think that the precedent should be “works the same as npm@5.3”, because that had some very rough and confusing edge cases we explicitly intended to avoid.


(system) #15

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.


(Kat Marchán) #16