fix: correctly ignore .gitignore when a .npmignore is present outside of the workspace tree #108
Conversation
nlf
force-pushed
the
nlf/ignore-gitignore-when-npmignore-present-out-of-tree
branch
from
e74eb95
to
b3e4cca
Compare
| for (const file of ['.npmignore', '.gitignore']) { | ||
| try { | ||
| const ignoreContent = fs.readFileSync(path.join(root, file), { encoding: 'utf8' }) | ||
| result += ignoreContent + '\n' | ||
| // break the loop immediately after concatting, this allows us to prioritize the | ||
| // .npmignore and discard the .gitignore if one exists | ||
| break | ||
| } catch (err) { |
There was a problem hiding this comment.
I reckon that, if I have a npmignore and gitignore at workspaces/foo but only a gitignore in workspaces (or any folder in between) or at the root level, I will endup with a result:
content of npmignore from foo
content of gitignore from workspace
content of gitignore from root
It seems odd to me to mix the npmignore and gitignore (ofc, if that the expected behavior, all good 👍 )
There was a problem hiding this comment.
that's the expected behavior, if a directory does not contain a .npmignore but does have a .gitignore then we respect the .gitignore
There was a problem hiding this comment.
👍 if that's the expected behaviour, then all's well.
I think you still need to reverse the result concatenation because you're crawling up the tree and not down, but I might be mistaken. nvm, I misread the path construction!
There was a problem hiding this comment.
we're crawling from the root deeper toward the workspace, you can see when we call readOutOfTreeIgnoreFiles here: https://github.com/npm/npm-packlist/blob/main/lib/index.js#L166 we're passing it opt.prefix which is the /root, and relpath to carry with your examples in #108 would be the string packages
the loop runs with root, then appends the next segment of the relpath (packages) and runs again, concatting this result to the end, this gives us the expected order of rules
https://coveord.atlassian.net/browse/CDX-1010 <!-- For Coveo Employees only. Fill this section. CDX-1010 --> ## Proposed changes I discussed with a developer from the npm team here npm/npm-packlist#106 and got a good understanding of their philosophy moving forward with the concept of workspaces and their multi-level ignore files. In short, npm will try to acquire an ignorefile at each directory level and concatenate them. The peculiarity (at least IMO), is that if it acquire a npmignore first, this will have no incidence for the acquisition of the other ignorefile. So, we can end up with a mix of `gitignore` and `npmignore`. I discussed this behavior with the npm team, and they are kinda stuck, so its their least worst option here, so we just have to adapt and make it work. This is what this PR is about. ## Testing - Ran `npm publish --dry-run` on every package and checked them out. ## Prerequisites - [x] npm/npm-packlist#108 - [ ] `npm-cli` released with ☝️
previously we were concatting the .npmignore to the .gitignore and using both, which is incorrect. this ensures if a .npmignore exists we only use those rules.
closes #106