fix: The default value of --ignore-files no longer causes build to create empty zip files when --source-dir is a sub directory of --artifacts-dir (#790)

Author: eight04

src/util/file-filter.js

@@ -7,24 +7,25 @@ import {createLogger} from './logger';
 
 const log = createLogger(__filename);
 
-// Use this function to mimic path.resolve without resolving to absolute path.
-export const normalizeResolve = (file: string): string => {
-  // normalize
-  file = path.normalize(file);
-
-  // trim trailing slash
-  if (path.parse(file).base && file.endsWith(path.sep)) {
-    return file.slice(0, -1);
+// check if target is a sub directory of src
+export const isSubPath = (src: string, target: string): boolean => {
+  const relate = path.relative(src, target);
+  // same dir
+  if (!relate) {
+    return false;
   }
-  return file;
+  if (relate === '..') {
+    return false;
+  }
+  return !relate.startsWith(`..${path.sep}`);
 };
 
 // FileFilter types and implementation.
 
 export type FileFilterOptions = {|
   baseIgnoredPatterns?: Array<string>,
   ignoreFiles?: Array<string>,
-  sourceDir?: string,
+  sourceDir: string,
   artifactsDir?: string,
 |};
 
@@ -33,7 +34,7 @@ export type FileFilterOptions = {|
  */
 export class FileFilter {
   filesToIgnore: Array<string>;
-  sourceDir: ?string;
+  sourceDir: string;
 
   constructor({
     baseIgnoredPatterns = [
@@ -48,6 +49,7 @@ export class FileFilter {
     sourceDir,
     artifactsDir,
   }: FileFilterOptions = {}) {
+    sourceDir = path.resolve(sourceDir);
 
     this.filesToIgnore = [];
     this.sourceDir = sourceDir;
@@ -56,7 +58,12 @@ export class FileFilter {
     if (ignoreFiles) {
       this.addToIgnoreList(ignoreFiles);
     }
-    if (artifactsDir) {
+    if (artifactsDir && isSubPath(sourceDir, artifactsDir)) {
+      artifactsDir = path.resolve(artifactsDir);
+      log.debug(
+        `Ignoring artifacts directory "${artifactsDir}" ` +
+        'and all its subdirectories'
+      );
       this.addToIgnoreList([
         artifactsDir,
         path.join(artifactsDir, '**', '*'),
@@ -65,26 +72,23 @@ export class FileFilter {
   }
 
   /**
-   *  Resolve relative path to absolute path if sourceDir is setted.
+   *  Resolve relative path to absolute path with sourceDir.
    */
-  resolve(file: string): string {
-    if (this.sourceDir) {
-      const resolvedPath = path.resolve(this.sourceDir, file);
-      log.debug(
-        `Resolved path ${file} with sourceDir ${this.sourceDir || ''} ` +
-        `to ${resolvedPath}`
-      );
-      return resolvedPath;
-    }
-    return normalizeResolve(file);
+  resolveWithSourceDir(file: string): string {
+    const resolvedPath = path.resolve(this.sourceDir, file);
+    log.debug(
+      `Resolved path ${file} with sourceDir ${this.sourceDir} ` +
+      `to ${resolvedPath}`
+    );
+    return resolvedPath;
   }
 
   /**
    *  Insert more files into filesToIgnore array.
    */
   addToIgnoreList(files: Array<string>) {
     for (const file of files) {
-      this.filesToIgnore.push(this.resolve(file));
+      this.filesToIgnore.push(this.resolveWithSourceDir(file));
     }
   }
 
@@ -99,7 +103,7 @@ export class FileFilter {
    * file in the folder that is being archived.
    */
   wantFile(filePath: string): boolean {
-    const resolvedPath = this.resolve(filePath);
+    const resolvedPath = this.resolveWithSourceDir(filePath);
     for (const test of this.filesToIgnore) {
       if (minimatch(resolvedPath, test)) {
         log.debug(

src/watcher.js

@@ -3,7 +3,6 @@ import Watchpack from 'watchpack';
 import debounce from 'debounce';
 
 import {createLogger} from './util/logger';
-import {FileFilter} from './util/file-filter';
 
 
 const log = createLogger(__filename);
@@ -19,7 +18,7 @@ export type OnSourceChangeParams = {|
   sourceDir: string,
   artifactsDir: string,
   onChange: OnChangeFn,
-  shouldWatchFile?: ShouldWatchFn,
+  shouldWatchFile: ShouldWatchFn,
 |};
 
 // NOTE: this fix an issue with flow and default exports (which currently
@@ -59,16 +58,12 @@ export type ProxyFileChangesParams = {|
   artifactsDir: string,
   onChange: OnChangeFn,
   filePath: string,
-  shouldWatchFile?: ShouldWatchFn,
+  shouldWatchFile: ShouldWatchFn,
 |};
 
 export function proxyFileChanges(
   {artifactsDir, onChange, filePath, shouldWatchFile}: ProxyFileChangesParams
 ): void {
-  if (!shouldWatchFile) {
-    const fileFilter = new FileFilter();
-    shouldWatchFile = (...args) => fileFilter.wantFile(...args);
-  }
   if (filePath.indexOf(artifactsDir) === 0 || !shouldWatchFile(filePath)) {
     log.debug(`Ignoring change to: ${filePath}`);
   } else {

tests/unit/test-cmd/test.build.js

@@ -224,6 +224,7 @@ describe('build', () => {
   it('asks FileFilter what files to include in the ZIP', () => {
     const zipFile = new ZipFile();
     const fileFilter = new FileFilter({
+      sourceDir: '.',
       baseIgnoredPatterns: ['**/background-script.js'],
     });
 
@@ -244,11 +245,11 @@ describe('build', () => {
 
   it('lets you rebuild when files change', () => withTempDir(
     (tmpDir) => {
-      const fileFilter = new FileFilter();
-      sinon.spy(fileFilter, 'wantFile');
-      const onSourceChange = sinon.spy(() => {});
       const sourceDir = fixturePath('minimal-web-ext');
       const artifactsDir = tmpDir.path();
+      const fileFilter = new FileFilter({sourceDir, artifactsDir});
+      sinon.spy(fileFilter, 'wantFile');
+      const onSourceChange = sinon.spy(() => {});
       return build({
         sourceDir, artifactsDir, asNeeded: true,
       }, {

tests/unit/test-util/test.file-filter.js

@@ -1,15 +1,20 @@
 /* @flow */
-import path from 'path';
-
 import {describe, it} from 'mocha';
 import {assert} from 'chai';
 
-import {FileFilter, normalizeResolve} from '../../../src/util/file-filter';
+import {FileFilter, isSubPath} from '../../../src/util/file-filter';
 
 describe('util/file-filter', () => {
 
+  const newFileFilter = (params = {}) => {
+    return new FileFilter({
+      sourceDir: '.',
+      ...params,
+    });
+  };
+
   describe('default', () => {
-    const defaultFilter = new FileFilter();
+    const defaultFilter = newFileFilter();
 
     it('ignores long XPI paths', () => {
       assert.equal(defaultFilter.wantFile('path/to/some.xpi'), false);
@@ -57,38 +62,47 @@ describe('util/file-filter', () => {
   describe('options', () => {
 
     it('override the defaults with baseIgnoredPatterns', () => {
-      const filter = new FileFilter({
+      const filter = newFileFilter({
         baseIgnoredPatterns: ['manifest.json'],
       });
       assert.equal(filter.wantFile('some.xpi'), true);
       assert.equal(filter.wantFile('manifest.json'), false);
     });
 
     it('add more files to ignore with ignoreFiles', () => {
-      const filter = new FileFilter({
+      const filter = newFileFilter({
         ignoreFiles: ['*.log'],
       });
       assert.equal(filter.wantFile('some.xpi'), false);
       assert.equal(filter.wantFile('some.log'), false);
     });
 
     it('ignore artifactsDir and its content', () => {
-      const filter = new FileFilter({
+      const filter = newFileFilter({
         artifactsDir: 'artifacts',
       });
       assert.equal(filter.wantFile('artifacts'), false);
       assert.equal(filter.wantFile('artifacts/some.js'), false);
     });
 
+    it('does not ignore an artifactsDir outside of sourceDir', () => {
+      const filter = newFileFilter({
+        artifactsDir: '.',
+        sourceDir: 'dist',
+      });
+      assert.equal(filter.wantFile('file'), true);
+      assert.equal(filter.wantFile('dist/file'), true);
+    });
+
     it('resolve relative path', () => {
-      const filter = new FileFilter({
+      const filter = newFileFilter({
         sourceDir: '/src',
         artifactsDir: 'artifacts',
         ignoreFiles: [
           'ignore-dir/', 'some.js', '**/some.log', 'ignore/dir/content/**/*',
         ],
       });
-      assert.equal(filter.wantFile('/src/artifacts'), false);
+      assert.equal(filter.wantFile('/src/artifacts'), true);
       assert.equal(filter.wantFile('/src/ignore-dir'), false);
       assert.equal(filter.wantFile('/src/ignore-dir/some.css'), true);
       assert.equal(filter.wantFile('/src/some.js'), false);
@@ -103,22 +117,17 @@ describe('util/file-filter', () => {
 
   });
 
-  describe('normalizeResolve', () => {
-    const paths = [
-      'file', 'dir/',
-      'path/to/file', 'path/to/dir/', 'path/to/../file', 'path/to/../dir/',
-      'path/to/dir/.', 'path/to/dir/..',
-    ];
-
-    it('mimic path.resolve', () => {
-      const src = '/src/';
-
-      paths.forEach((file) => {
-        assert.equal(
-          path.resolve(src, file),
-          path.join(path.resolve(src), normalizeResolve(file))
-        );
-      });
+  describe('isSubPath', () => {
+    it('test if target is a sub directory of src', () => {
+      assert.equal(isSubPath('dist', '.'), false);
+      assert.equal(isSubPath('.', 'artifacts'), true);
+      assert.equal(isSubPath('.', '.'), false);
+      assert.equal(isSubPath('/src/dist', '/src'), false);
+      assert.equal(isSubPath('/src', '/src/artifacts'), true);
+      assert.equal(isSubPath('/src', '/src'), false);
+      assert.equal(isSubPath('/firstroot', '/secondroot'), false);
+      assert.equal(isSubPath('/src', '/src/.dir'), true);
+      assert.equal(isSubPath('/src', '/src/..dir'), true);
     });
   });
 

tests/unit/test.watcher.js

@@ -31,6 +31,7 @@ describe('watcher', () => {
             sourceDir: tmpDir.path(),
             artifactsDir,
             onChange,
+            shouldWatchFile: () => true,
           });
         })
         .then((watcher) => {
@@ -56,6 +57,7 @@ describe('watcher', () => {
     const defaults = {
       artifactsDir: '/some/artifacts/dir/',
       onChange: () => {},
+      shouldWatchFile: () => true,
     };
 
     it('proxies file changes', () => {
@@ -103,16 +105,6 @@ describe('watcher', () => {
 
     });
 
-    it('filters out commonly unwanted files by default', () => {
-      const conf = {
-        ...defaults,
-        shouldWatchFile: undefined,
-        onChange: sinon.spy(() => {}),
-      };
-      proxyFileChanges({...conf, filePath: '/somewhere/.git'});
-      assert.equal(conf.onChange.called, false);
-    });
-
   });
 
 });