Fix yarn pack to always include the file in the "main" field
#3092
Conversation
| @@ -3,5 +3,5 @@ | |||
| "version": "1.0.0", | |||
| "main": "index.js", | |||
| "license": "MIT", | |||
| "files": ["index.js", "a.js", "b.js"] | |||
| "files": ["a.js", "b.js"] | |||
There was a problem hiding this comment.
could you add index.js to main filed?
|
Hi @bestander, thanks for the quick review! Could you elaborate on your comments? I'm not sure I understand what needs to be changed, as index.js is already in the main field of the test fixture (here), and it is tested to be present in the tarball here. |
|
|
||
| // inlude required files | ||
| let filters: Array<IgnoreFilter> = NEVER_IGNORE.slice(); | ||
| // include default filters unless `files` is used | ||
| if (!onlyFiles) { | ||
| filters = filters.concat(DEFAULT_IGNORE); | ||
| } | ||
| if (main) { |
There was a problem hiding this comment.
sure, @josephfrazier.
You've added a new condition checking for main in package.json.
But have not added a test that covers this condition.
In __tests__/fixtures/pack/files-include-mandatory/package.json you probably wanted to add main field with index.js and not have it in files?
There was a problem hiding this comment.
Hmm, still not sure I understand. In __tests__/fixtures/pack/files-include-mandatory/package.json, the main field is already index.js. The point of this change is to make sure that index.js doesn't have to be in files in order for the packed tarball to contain index.js.
I split my changes into two commits - test first, fix second - which should help demonstrate how the new condition is covered by the test. If you run the following at each commit, you'll see how the tarball changes (this is also covered by yarn build && yarn test -- __tests__/commands/pack.js, but I pushed the two commits too quickly, and the CI for the first commit seems to have run against the changes in the second commit):
yarn build && cd __tests__/fixtures/pack/files-include-mandatory && ../../../../bin/yarn.js pack && tar tvf *tgz | grep index.js; cd -
38e6ea9 to
89ac230
Compare
This is for compatibility with npm, which [specifies] that: > Certain files are always included, regardless of settings: > * package.json > * README > * CHANGES / CHANGELOG / HISTORY > * LICENSE / LICENCE > * NOTICE > * The file in the "main" field [specifies]: https://docs.npmjs.com/files/package.json#files
This is for compatibility with npm, which [specifies] that: > Certain files are always included, regardless of settings: > * package.json > * README > * CHANGES / CHANGELOG / HISTORY > * LICENSE / LICENCE > * NOTICE > * The file in the "main" field [specifies]: https://docs.npmjs.com/files/package.json#files
5105321 to
09067d3
Compare
|
Thanks, looks great |
Summary
This is for compatibility with npm, which specifies that:
Test plan
I changed the
files-include-mandatorytest to exclude "index.js" fromfiles, and ensure that it still gets included, since it is themainfile.