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

pack: include contents of directories in files field #3175

Merged
merged 13 commits into from
Apr 28, 2017
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
14 changes: 9 additions & 5 deletions __tests__/commands/pack.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export async function getFilesFromArchive(source, destination): Promise<Array<st
.on('error', reject);
});
await unzip;
const files = await fs.readdir(destination);
const files = (await fs.walk(destination)).map(({relative}) => relative);
return files;
}

Expand All @@ -115,10 +115,14 @@ test.concurrent('pack should inlude all files listed in the files array', (): Pr
path.join(cwd, 'files-include-v1.0.0.tgz'),
path.join(cwd, 'files-include-v1.0.0'),
);
const expected = ['index.js', 'a.js', 'b.js'];
expected.forEach((filename) => {
expect(files.indexOf(filename)).toBeGreaterThanOrEqual(0);
});
expect(files.sort()).toEqual([
'a.js',
'b.js',
'dir',
path.join('dir', 'nested.js'),
'index.js',
'package.json',
]);
});
});

Expand Down
2 changes: 2 additions & 0 deletions __tests__/fixtures/pack/files-include/dir/nested.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/* @flow */
console.log('hello world');
2 changes: 1 addition & 1 deletion __tests__/fixtures/pack/files-include/package.json
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": ["index.js", "a.js", "b.js", "dir/"]
}
73 changes: 22 additions & 51 deletions src/cli/commands/pack.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {MessageError} from '../../errors.js';

const zlib = require('zlib');
const path = require('path');
const tar = require('tar-stream');
const tar = require('tar-fs');
const fs2 = require('fs');

const IGNORE_FILENAMES = [
Expand Down Expand Up @@ -54,18 +54,6 @@ const NEVER_IGNORE = ignoreLinesToRegex([
'!/+(changes|changelog|history)*',
]);

function addEntry(packer: any, entry: Object, buffer?: ?Buffer): Promise<void> {
return new Promise((resolve, reject) => {
packer.entry(entry, buffer, function(err) {
if (err) {
reject(err);
} else {
resolve();
}
});
});
}

export async function pack(config: Config, dir: string): Promise<stream$Duplex> {
const pkg = await config.readRootManifest();
const {bundledDependencies, files: onlyFiles} = pkg;
Expand Down Expand Up @@ -94,6 +82,7 @@ export async function pack(config: Config, dir: string): Promise<stream$Duplex>
];
lines = lines.concat(
onlyFiles.map((filename: string): string => `!${filename}`),
onlyFiles.map((filename: string): string => `!${path.join(filename, '**')}`),
);
const regexes = ignoreLinesToRegex(lines, '.');
filters = filters.concat(regexes);
Expand Down Expand Up @@ -126,46 +115,28 @@ export async function pack(config: Config, dir: string): Promise<stream$Duplex>
// apply filters
sortFilter(files, filters, keepFiles, possibleKeepFiles, ignoredFiles);

const packer = tar.pack();
const compressor = packer.pipe(new zlib.Gzip());

await addEntry(packer, {
name: 'package',
type: 'directory',
const packer = tar.pack(config.cwd, {
ignore: (name) => {
const relative = path.relative(config.cwd, name);
// Don't ignore directories, since we need to recurse inside them to check for unignored files.
if (fs2.lstatSync(name).isDirectory()) {
const isParentOfKeptFile = Array.from(keepFiles).some((name) =>
!path.relative(relative, name).startsWith('..'));
return !isParentOfKeptFile;
}
// Otherwise, ignore a file if we're not supposed to keep it.
return !keepFiles.has(relative);
},
map: (header) => {
const suffix = header.name === '.' ? '' : `/${header.name}`;
header.name = `package${suffix}`;
delete header.uid;
delete header.gid;
return header;
},
});

for (const name of keepFiles) {
const loc = path.join(config.cwd, name);
const stat = await fs.lstat(loc);

let type: ?string;
let buffer: ?Buffer;
let linkname: ?string;
if (stat.isDirectory()) {
type = 'directory';
} else if (stat.isFile()) {
buffer = await fs.readFileRaw(loc);
type = 'file';
} else if (stat.isSymbolicLink()) {
type = 'symlink';
linkname = await fs.readlink(loc);
} else {
throw new Error();
}

const entry = {
name: `package/${name}`,
size: stat.size,
mode: stat.mode,
mtime: stat.mtime,
type,
linkname,
};

await addEntry(packer, entry, buffer);
}

packer.finalize();
const compressor = packer.pipe(new zlib.Gzip());

return compressor;
}
Expand Down
2 changes: 2 additions & 0 deletions src/util/filter.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ export function matchesFilter(filter: IgnoreFilter, basename: string, loc: strin
}
return filter.regex.test(loc) ||
filter.regex.test(`/${loc}`) ||
filter.regex.test(`\\${loc}`) ||

Choose a reason for hiding this comment

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

why not using path.sep?

Choose a reason for hiding this comment

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

uh, wait it produces \something, and it's interpreted as an escape token... Sorry..

maybe path.sep.replace('\\', '\\\\')?

Choose a reason for hiding this comment

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

The problem I see with keeping both / and \\ is that the backslash has a meaning in unix systems, so false-positives could occur.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The argument to test is a string rather than a RegExp pattern, so it shouldn't be interpreting backslashes as the escape character in this context. Note that the following expression (on Windows) should produce a string whose first character is a backslash:

`${path.sep}${loc}`

That said, I'm not sure why c2df043 seemed to make things worse, test-wise (an additional test failed)

filter.regex.test(basename);
}

Expand All @@ -123,6 +124,7 @@ export function ignoreLinesToRegex(lines: Array<string>, base: string = '.'): Ar

// remove trailing slash
pattern = removeSuffix(pattern, '/');
pattern = removeSuffix(pattern, '\\');

const regex: ?RegExp = minimatch.makeRe(pattern, {nocase: true});

Expand Down