Test if entries are directories before mandatorily including them #10445
Conversation
|
This looks good, but I'd like a test before landing it. Is that something you could add, Luke? Thanks for putting this together! |
|
Yeah, I could add a test for this. Would it be best to create a new test specifically for I wasn't sure since I think |
|
I think an integration test where you create the tree structure you're trying to pack / filter, and then looking at the results after you install that packed tarball would do the trick. That code is not super friendly to unit testing right now, but we're in the process of cleaning up the test suite, and we might have bright ideas on how to simplify the integration tests some in the process. We could probably help you write the tests, but it might take us a month or so, as we've got our hands full at the moment. |
This matches the behavior in fstream-npm@1.0.6 from d82ff81.
|
Would it be possible to amend the npm v3.4.0 release notes (https://github.com/npm/npm/releases/tag/v3.4.0) to remove the note about this bug having been fixed there? This caused us some issues earlier. |
|
Just ran into this, anything I can do to help land this? |
|
@glenjamin I never found time to add a test for this. If you wanted to add a commit with tests for this behavior, I imagine this would be ready to merge. |
|
Yes, with a test case, this would be ready to land. Feel free to submit a new PR to replace this one if you do that, @glenjamin! And thanks to you and @lukekarrys! |
|
Given #11995, can we close this PR? |
|
Yep, I'll close. |
Most notably, the package 'history' will not longer be included in the packages of anything which depends on it. Likewise with packages named changelog or license. This was fixed in fstream-npm@1.0.6 from via d82ff81, but for some reason the internal tar code duplicated this logic. Removing the duplication passes all the tests, so seems prudent. While fixing this bug, it was noticed that the `files-and-ignores` tests were giving false positives because they use `npm install` instead of only `npm pack`, the tests have been modified to avoid this. See also npm#9642, npm#10445, npm#11995
Most notably, the package 'history' will not longer be included in the packages of anything which depends on it. Likewise with packages named changelog or license. This was fixed in fstream-npm@1.0.6 from via d82ff81, but for some reason the internal tar code duplicated this logic. Removing the duplication passes all the tests, so seems prudent. While fixing this bug, it was noticed that the `files-and-ignores` tests were giving false positives because they use `npm install` instead of only `npm pack`, the tests have been modified to avoid this. See also #9642, #10445, #11995 Credit: @glenjamin Reviewed-By: @zkat PR-URL: #11995
This matches the behavior in fstream-npm@1.0.6 from d82ff81.
I was tempted to completely remove the
applyIgnoresmethod (which is the only thingBundledPackerextends fromfstream-npm), but it seems like there is slightly different logic in how bundled dependencies are packaged innpmvsfstream-npm. The bundled dependencies logic was the only thing I could find thatnpmdoes in addition tofstream-npm, everything else was additional logic fromfstream-npm.Here's a full diff of the two functions: https://gist.github.com/lukekarrys/4bf53ef42ac41d395a32
Ref #9642