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

Fix yarn pack to always include the file in the "main" field #3092

Merged
merged 2 commits into from
Apr 21, 2017
Merged

Fix yarn pack to always include the file in the "main" field #3092

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f3dd1d6
Test that `yarn pack` always includes the file in the "main" field
josephfrazier Apr 11, 2017
09067d3
Fix `yarn pack` to always include the file in the "main" field
josephfrazier Apr 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion __tests__/commands/pack.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ test.concurrent('pack should include mandatory files not listed in files array i
path.join(cwd, 'files-include-mandatory-v1.0.0.tgz'),
path.join(cwd, 'files-include-mandatory-v1.0.0'),
);
const expected = ['package.json', 'readme.md', 'license', 'changelog'];
const expected = ['package.json', 'index.js', 'readme.md', 'license', 'changelog'];
expected.forEach((filename) => {
expect(files.indexOf(filename)).toBeGreaterThanOrEqual(0);
});
Expand Down
2 changes: 1 addition & 1 deletion __tests__/fixtures/pack/files-include-mandatory/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Copy link
Member

Choose a reason for hiding this comment

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

could you add index.js to main filed?

}
5 changes: 4 additions & 1 deletion src/cli/commands/pack.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,17 @@ function addEntry(packer: any, entry: Object, buffer?: ?Buffer): Promise<void> {

export async function pack(config: Config, dir: string): Promise<stream$Duplex> {
const pkg = await config.readRootManifest();
const {bundledDependencies, files: onlyFiles} = pkg;
const {bundledDependencies, main, files: onlyFiles} = pkg;

// 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) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@josephfrazier josephfrazier Apr 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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 -

filters = filters.concat(ignoreLinesToRegex(['!/' + main]));
}

// include bundledDependencies
if (bundledDependencies) {
Expand Down
1 change: 1 addition & 0 deletions src/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ export type Manifest = {

deprecated?: string,
files?: Array<string>,
main?: string,
};

//
Expand Down