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

fix: correctly ignore .gitignore when a .npmignore is present outside of the workspace tree #108

Merged
merged 1 commit into from
May 31, 2022
Merged

fix: correctly ignore .gitignore when a .npmignore is present outside of the workspace tree #108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,13 @@ const glob = require('glob')
const globify = pattern => pattern.split('\\').join('/')

const readOutOfTreeIgnoreFiles = (root, rel, result = '') => {
for (const file of ['.gitignore', '.npmignore']) {
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) {
Comment on lines +37 to 44
Copy link

Choose a reason for hiding this comment

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

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 👍 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's the expected behavior, if a directory does not contain a .npmignore but does have a .gitignore then we respect the .gitignore

Copy link

Choose a reason for hiding this comment

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

👍 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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

// we ignore ENOENT errors completely because we don't care if the file doesn't exist
// but we throw everything else because failing to read a file that does exist is
Expand Down
64 changes: 64 additions & 0 deletions test/workspace.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,3 +109,67 @@ t.test('packing a workspace root does not include children', async (t) => {
'workspaces/foo/package.json',
])
})

t.test('.gitignore is discarded if .npmignore exists outside of tree', async (t) => {
const root = t.testdir({
'package.json': JSON.stringify({
name: 'workspace-root',
version: '1.0.0',
main: 'root.js',
workspaces: ['./workspaces/foo'],
}),
'root.js': `console.log('hello')`,
'.gitignore': 'dont-ignore-me',
'.npmignore': 'only-ignore-me',
'dont-ignore-me': 'should not be ignored',
'only-ignore-me': 'should be ignored',
workspaces: {
'.gitignore': 'dont-ignore-me-either',
'.npmignore': 'ignore-me-also',
'dont-ignore-me': 'should not be ignored',
'dont-ignore-me-either': 'should not be ignored',
'only-ignore-me': 'should be ignored',
'ignore-me-also': 'should be ignored',
foo: {
'package.json': JSON.stringify({
name: 'workspace-child',
version: '1.0.0',
main: 'child.js',
}),
'child.js': `console.log('hello')`,
'dont-ignore-me': 'should not be ignored',
'dont-ignore-me-either': 'should not be ignored',
'only-ignore-me': 'should be ignored',
'ignore-me-also': 'should also be ignored',
},
},
})

const workspacePath = path.join(root, 'workspaces', 'foo')
// this simulates what it looks like when a user does i.e. npm pack -w ./workspaces/foo
const files = await packlist({
path: workspacePath,
prefix: root,
workspaces: [workspacePath],
})
t.same(files, [
'dont-ignore-me',
'dont-ignore-me-either',
'child.js',
'package.json',
])

// here we leave off workspaces to satisfy coverage
const secondFiles = await packlist({
path: workspacePath,
prefix: root,
})
t.same(secondFiles, [
'dont-ignore-me',
'dont-ignore-me-either',
'ignore-me-also',
'only-ignore-me',
'child.js',
'package.json',
])
})