ignore artifacts directory when building XPI

[iamVP7]

Fixes #291

Explanation as follow

So first eliminating web-ext-artifacts dir

checking whether artifactsDir contains the string web-ext-artifacts
if it contains then the values before that string is sliced.

if it is not present
checking whether artifactsDir contains '/' at last like ./viswa/test/
if it contains then using path.join method to remove(./) and the slice to remove /

if last character is not '/' like ./viswa/test
then using path.join method to remove(./)

[kumar303]

I think this would be easier if you made a separate array, like, this.filePathsToIgnore and then checked the exact paths in a loop separate from the minimatch loop. Example:

wantFile(path: string): boolean {
  for (const pathToIgnore of this.filePathsToIgnore) {
    if (path === pathToIgnore) {
      return false;
    }
  }
  for (const test of this.filesToIgnore) {
    if (minimatch(path, test)) {
      // no change to this...
    }
  }
}

[iamVP7]

Wont Having two For loops, increase time complexity. Lets take some path which will get failed in if check first For loop, and then it comes to second for loop for checking. When it fails in second also i feel we will increase the run time.

I had a thought to remove the below lines from constructor and make it as a separate method so we can increase readability.

var eliminateArtifactDir = new String();
var buf = artifactsDir;
if(typeof buf !== 'undefined' && buf.indexOf('web-ext-artifacts') != -1){
eliminateArtifactDir = buf.slice(buf.indexOf('web-ext-artifacts'));
}
else if(typeof buf !== 'undefined' && buf.slice(-1) === '/'){
eliminateArtifactDir = path.join(buf.slice(0,-1));
}
else if(typeof buf !== 'undefined'){
eliminateArtifactDir = path.join(buf);
}

[kumar303]

I don't like the complexity in your current patch, there are too many steps taken to convert the path to a minimatch value. If we introduce another for-loop I don't think it will pose a performance problem. The minimatch pattern matcher is probably slower than a straight path check anyway.

[kumar303]

these could paths could be files or directories so I think you should name it something more like this.filePathsToIgnore.

[kumar303]

You need to rebase this on master because of conflicts

[kumar303]

This is looking better. The next step is that you need to add a test for the new FileFilter functionality. It can go in https://github.com/mozilla/web-ext/blob/master/tests/test-cmd/test.build.js#L174

[kumar303]

Accepting artifactsDir as an argument is not necessary. The FileFilter doesn't need to know about the artifacts directory, it just needs to know which file paths to ignore.

Instead of calling the FileFilter like this:

const filter = new FileFilter(artifactsDir);

you could call it like this:

const filter = new FileFilter({filePathsToIgnore: [path.resolve(artifactsDir)]});

This makes the FileFilter more flexible; you could pass in as many file paths to ignore as necessary.

[kumar303]

you will also need to look at all the places that FileFilter() is called and update that code

[coveralls]

Coverage decreased (-0.7%) to 99.334% when pulling c34bb5f on iamVP7:master into 830065b on mozilla:master.

[coveralls]

Coverage decreased (-0.7%) to 99.334% when pulling eb8e139 on iamVP7:master into 830065b on mozilla:master.

[coveralls]

Coverage decreased (-0.6%) to 99.373% when pulling 7585db1 on iamVP7:master into 74e9bba on mozilla:master.

[coveralls]

Coverage decreased (-0.3%) to 99.687% when pulling 05984c6 on iamVP7:master into 74e9bba on mozilla:master.

[coveralls]

Coverage remained the same at 100.0% when pulling 97b13a3 on iamVP7:master into 74e9bba on mozilla:master.

[kumar303]

Please delete this file. It looks like maybe your editor created it automatically.

[kumar303]

I don't think it's necessary to construct a default filter that ignores artifact directories here. The build command already takes responsibility for this.

[coveralls]

Coverage remained the same at 100.0% when pulling 43cbb08 on iamVP7:master into 74e9bba on mozilla:master.

[kumar303]

To avoid confusion, I'd like to see the old FileFilter parameter, filesToIgnore, renamed to filePatternsToIgnore. This will make it more clear that the old parameter contains patterns and your new parameter contains file paths. This will require you to update all calls to FileFilter throughout the src and tests directories.

[kumar303]

I'd like to see one other test that makes sure the build command is configuring the filter correctly. It would require cloning the fixture directory, building the extension, unzipping the extension, and making sure the extension doesn't include the artifacts directory. Here is some code that might work for it:

import copyDir from 'copy-dir';
import {promisify} from '../../src/util/es6-modules';

describe('build', () => {

  it('ignores the artifacts dir', () => {
    const zipFile = new ZipFile();

    return withTempDir(
      (tmpDir) => {
        const sourceDir = path.join(tmpDir.path(), 'source-dir');
        const artifactsDir = path.join(sourceDir, 'web-ext-artifacts');
        const copyDirAsPromised = promisify(copyDir);
        return copyDirAsPromised(fixturePath('minimal-web-ext'), sourceDir)
          .then(() => build({sourceDir, artifactsDir}))
          .then((buildResult) => zipFile.open(buildResult.extensionPath))
          .then(() => zipFile.extractFilenames())
          .then((fileNames) => {
            assert.notInclude(fileNames, artifactsDir);
          });
      }
    );
  });

});

[coveralls]

Coverage decreased (-0.2%) to 99.844% when pulling 94c1906 on iamVP7:master into fa27fb0 on mozilla:master.

[kumar303]

Hi @iamVP7 . We landed a big change on master that switches to using the async/await strategy instead of promises. You'll have to re-apply your patch using the new strategy. I would suggest leaving this PR open (as reference) and starting a new PR in a new branch. Let me or @rpl know if you need further guidance.

If you ran out of time to work on this feature, that's totally understandable too. Thanks for giving it a shot!

[kumar303]

I received an email that this patch is on hold until further notice. I'm closing it in case another contributor wants to pick it up. Thanks for your interest in web-ext !