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

fix(pack): higher priority to .npmignore over .gitignore on windows #5979

Merged
merged 1 commit into from
Jul 19, 2018

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Jun 12, 2018

Summary

This fixes yarn pack not ignoring .gitignore when .npmignore is avaialable on Windows. Initial fix was done in #3538, but it used string concatenation to join paths which make it unfortunately fail for Windows which uses different path separator than other systems. It was reported as not fixed in #685 (comment) (OS: Windows) in initial issue after issue was closed as fixed.

Test plan

First I tested if tarball created by yarn pack is correct (so it ignores .gitignore when I have .npmignore).

To demonstrate here, I added extra console output here https://github.com/yarnpkg/yarn/blob/master/src/cli/commands/pack.js#L89 logging dotIgnoreFiles and here's before / after:

Before:

D:\dev\yarn-test>yarn pack
yarn pack v1.7.0
dotIgnoreFiles [ { relative: '.gitignore',
    basename: '.gitignore',
    absolute: 'D:\\dev\\yarn-test\\.gitignore',
    mtime: 1528836250698 },
  { relative: '.npmignore',
    basename: '.npmignore',
    absolute: 'D:\\dev\\yarn-test\\.npmignore',
    mtime: 1528836254147 } ]
success Wrote tarball to "D:\\dev\\yarn-test\\yarn-test-v1.0.0.tgz".
Done in 0.45s.

After:

D:\dev\yarn-test>yarn pack
yarn pack v1.7.0
dotIgnoreFiles [ { relative: '.npmignore',
    basename: '.npmignore',
    absolute: 'D:\\dev\\yarn-test\\.npmignore',
    mtime: 1528836254147 } ]
success Wrote tarball to "D:\\dev\\yarn-test\\yarn-test-v1.0.0.tgz".
Done in 0.06s.

.gitignore is correctly omitted if .npmignore is present

Copy link
Contributor

@hulkish hulkish left a comment

Choose a reason for hiding this comment

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

LGTM

@arcanis
Copy link
Member

arcanis commented Jul 19, 2018

Thanks! The change looks safe so I'm going to merge it, but it would be great if you could add a test in __tests__, @pieh

@arcanis arcanis merged commit 8a40461 into yarnpkg:master Jul 19, 2018
@arcanis arcanis mentioned this pull request Jul 25, 2018
@pieh
Copy link
Contributor Author

pieh commented Jul 27, 2018

Let me know if those tests are still needed after #6149. I had started on something like pieh@a269136 but didn't like changing filterOverridenGitignores function signature for passing path.posix.join or path.win32.join just for tests

@arcanis
Copy link
Member

arcanis commented Jul 27, 2018

Yep, it would be nice to have a new test that would ensure that the .npmignore always has precedence over .gitignore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants