feat: Allow negation in --ignore-files #1348

Gozala · 2018-07-19T20:57:53Z

This change swaps minimatch for multimatch to allow opt-in inclusion subset of node_modules

coveralls · 2018-07-19T21:16:24Z

Coverage remained the same at 100.0% when pulling 7d48421 on Gozala:ignore-negation into bc4eb1c on mozilla:master.

Rob--W · 2018-07-20T10:10:40Z

We considered the removal of **/node_modules/**/*' (because **/node_modules') is already in the list):

web-ext/src/util/file-filter.js

Line 46 in 0fd2dd3

'**/node_modules/**/*',

Would that solve the original issue?

If not, and this change is necessary, can you include a unit test that fails before this PR and passes with your proposed change?

rpl · 2018-07-20T15:12:48Z

@Rob--W looking at the following code:

web-ext/src/util/file-filter.js

Lines 107 to 113 in bc4eb1c

    
           for (const test of this.filesToIgnore) { 
        
             if (minimatch(resolvedPath, test)) { 
        
               log.debug( 
        
                 `FileFilter: ignoring file ${resolvedPath} (it matched ${test})`); 
        
               return false; 
        
             } 
        
           }

removing **/node_modules/**/* does not seem to be enough, because we are going to filter out the file as soon as we find the first pattern that matches.

I (very briefly) looked to the multimatch package and it seems that what it basically does is to "extend minimatch to support matching on a set of patterns", and it depends from minimatch ^3.0.0.

@Gozala I agree with @Rob--W that some additional unit tests are needed for this change.
(I would also like if you could revert some of the changes to the indentation and spacing, to be consistent with how the file was initially, I guess that most of those changes may have been done automatically by prettier executed automatically by the editor on save).

Gozala · 2018-07-20T17:15:10Z

Would that solve the original issue?

Nope it would not as intention is to include some libraries installed through npm by manually whitelisting them.

If not, and this change is necessary, can you include a unit test that fails before this PR and passes with your proposed change?

I will update this pull with to include that.

@Gozala I agree with @Rob--W that some additional unit tests are needed for this change.
(I would also like if you could revert some of the changes to the indentation and spacing, to be consistent with how the file was initially, I guess that most of those changes may have been done automatically by prettier executed automatically by the editor on save).

Thanks for pointing that out. I thought I've disabled prettier to avoid that but I guess it still end up changing things.

Gozala · 2018-07-20T19:19:19Z

@Rob--W @rpl

I have updated this pull request such that:

First commit contains test that illustrates issues Allow including some files from node_modules #1347 and fails
Second commit contains changes that add support for negation in --ignore-files and passes test introduced in first commit.
Got rid of all formatting changes that were present previously.

Rob--W

Looks good to me. I only have two minor comments.

Rob--W · 2018-07-20T19:34:20Z

src/util/file-filter.js

-      this.filesToIgnore.push(this.resolveWithSourceDir(file));
+      if (file.charAt(0) === NEGATION) {
+        const resolvedFile = this.resolveWithSourceDir(file.substr(1));
+        this.filesToIgnore.push(`${NEGATION}${resolvedFile}`);


I think that the code is as readable if you use ! instead of ${NEGATION}.

Rob--W · 2018-07-20T19:34:27Z

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

+        filter.wantFile('/src/node_modules/libdweb/src/lib.js'),
+        true);
+      assert.equal(
+        filter.wantFile('/src/node_modules/libdweb/src/sub/lib.js'),


Please add a test for /src/node_modules/some/other/node_modules/index.js (I guess that the path is included because of the negation, but I would not be surprised if it got ignored anyway because of the deeper node_modules/.

Gozala · 2018-07-20T20:23:05Z

@Rob--W updated pull request to address your comments above.

Rob--W

r+ if tests pass

Gozala · 2018-07-20T21:21:25Z

hmm, I think travis is failing because of pull request title

- Swaps minimatch for multimatch to allow opt-in inclusion of node_modules subset - Fix test introduced in previous commit - Fix mozilla#1347

Gozala · 2018-07-20T21:52:28Z

@Rob--W Alright it's all green now, as just a pull request title. Can you please merge it in ?

Thanks

rpl

So, given the final goal of this PR (to make web-ext build --ignore-files '!node_modules/somepkg/somepattern' to include the given file (or files) in the resulting generated zip file), it seems to me that (despite the change applied by this pull request to FileFilter), zipDir is still going to exclude those files because it calls the 'filter' callback (

web-ext/src/cmd/build.js

Line 128 in 1cd88a1

filter: (...args) => fileFilter.wantFile(...args),

, https://github.com/jsantell/node-zip-dir#options) for every directory before descending it, and given that '/somepath/myproj/node_modules' doesn't match '!node_modules/somepkg/somepattern', zipDir is not going to descend into that directory.

@Gozala I think that it would be a good idea to add the following additional test case in test.build.js:

  it('zips a package and includes a file from a negated filter', () => {
    const zipFile = new ZipFile();

    return withTempDir(
      (tmpDir) =>
        build({
          sourceDir: fixturePath('minimal-web-ext'),
          artifactsDir: tmpDir.path(),
          ignoreFiles: [
            '!node_modules/pkg1/file1.txt',
          ],
        })
          .then((buildResult) => {
            assert.match(buildResult.extensionPath,
                         /minimal_extension-1\.0\.zip$/);
            return buildResult.extensionPath;
          })
          .then((extensionPath) => zipFile.open(extensionPath))
          .then(() => zipFile.extractFilenames())
          .then((fileNames) => {
            fileNames.sort();
            assert.deepEqual(fileNames, [
              'background-script.js', 'manifest.json',
              'node_modules/',
              'node_modules/pkg1/',
              'node_modules/pkg1/file1.txt',
            ]);
            return zipFile.close();
          })
    );
  });

and to add the following empty files to that fixture extension:

tests/fixtures/minimal-web-ext/.private-file1.txt
tests/fixtures/minimal-web-ext/node_modules/pkg1/file1.txt
tests/fixtures/minimal-web-ext/node_modules/pkg2/file2.txt

This additional files in the fixture would ensure that we still ignore those files by default, because of the existing test case:

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

Lines 31 to 54 in 1cd88a1

    
           it('zips a package', () => { 
        
             const zipFile = new ZipFile(); 
        
             return withTempDir( 
        
               (tmpDir) => 
        
                 build({ 
        
                   sourceDir: fixturePath('minimal-web-ext'), 
        
                   artifactsDir: tmpDir.path(), 
        
                 }) 
        
                   .then((buildResult) => { 
        
                     assert.match(buildResult.extensionPath, 
        
                                  /minimal_extension-1\.0\.zip$/); 
        
                     return buildResult.extensionPath; 
        
                   }) 
        
                   .then((extensionPath) => zipFile.open(extensionPath)) 
        
                   .then(() => zipFile.extractFilenames()) 
        
                   .then((fileNames) => { 
        
                     fileNames.sort(); 
        
                     assert.deepEqual(fileNames, 
        
                                      ['background-script.js', 'manifest.json']); 
        
                     return zipFile.close(); 
        
                   }) 
        
             ); 
        
           });

and the new 'zips a package and includes a file from a negated filter' test case will confirm if the new additional behaviors related to the negated filters are actually working as expected.

My guess is that with the current implemented changes there are still at least three additional negated patterns that are actually needed to force web-ext build to include that file:

'!node_modules'
'!node_modules/pkg1'
'!node_modules/pkg1/file1.txt'

and so the command line would be:

web-ext build -i '!node_modules/' -i '!node_modules/pkg1/' -i '!node_modules/pkg1/file1.txt'

Gozala · 2018-07-23T20:40:05Z

@rpl Thanks for thorough review, you were right that:

My intention was to use --ignore-files !node_modules/pkg1/** to opt-into including pkg1 into bundle.
And that patch as it is would not accomplish that. Instead I'd need to do -i '!node_modules' -i '!node_modules/pkg1' -i '!node_modules/pkg1/**'

Which is unfortunate, but give how [node-zip-dir](https://github.com/jsantell/node-zip-dir#options works) I am not sure there is a better way. Only other option I can imagine is add negation rules for all parent dirs automatically, so say ignoreFiles is ['!node_modules/pkg1/**'] expand that into ['!node_modules', '!node_modules/pkg1, '!node_modules/pkg1/**] automatically, but that isn't ideal either given that:

You might end up with zip containing bunch of empty directories.
You might have more evolved patterns like /foo/**/bar/*.js which would be difficult to translate.

Given the constraints I think manually negating parent dirs is probably good compromise as it still can achieve goal of including specific directories without introducing too much complexity.

Probably best solution would be to change @jsantell/node-zip-dir so that it understands globs itself, but again I think this pull request is probably a good enough compromise.

P.S. I have added test you've conveniently provided in the comment, except I modified it slightly to capture how ignore negation should be used to actually include desired package.

Gozala · 2018-07-23T20:42:56Z

@rpl also it seems that pull did not picked up on requested changes being addressed and I don't seem to see a way to re-trigger another review request, so just posting this comment instead.

rpl · 2018-07-24T19:33:20Z

Which is unfortunate, but give how [node-zip-dir](https://github.com/jsantell/node-zip-dir#options works) I am not sure there is a better way. Only other option I can imagine is add negation rules for all parent dirs automatically, so say ignoreFiles is ['!node_modules/pkg1/'] expand that into ['!node_modules', '!node_modules/pkg1, '!node_modules/pkg1/] automatically, but that isn't ideal either given that:

You might end up with zip containing bunch of empty directories.

You might have more evolved patterns like /foo/**/bar/*.js which would be difficult to translate.

@Gozala Yeah, I definitely agree, I definitely prefer to avoid adding implicitly a bunch of empty directories to workaround that (and yeah, it wouldn't work with more complex negated patterns, which is one more reason to avoid the workaround).

Given the constraints I think manually negating parent dirs is probably good compromise as it still can achieve goal of including specific directories without introducing too much complexity.

👍 I mainly wanted to be sure that we double-checked the actual behavior and agreed that it would be good enough for now.

We will need to document the behavior expected when negated pattern are used in the ignore file params or configs in any case and we can mention this behavior in those additions to the docs (I added the "needs: docs" label on both this PR and the original issue, to make sure that we document this on the MDN doc pages for the web-ext tool).

Probably best solution would be to change @jsantell/node-zip-dir so that it understands globs itself, but again I think this pull request is probably a good enough compromise.

Sounds good to me too, we can defer that for further follow ups.

Gozala force-pushed the ignore-negation branch from f8a8509 to 82be305 Compare July 19, 2018 21:16

Gozala force-pushed the ignore-negation branch from 82be305 to 5ed2362 Compare July 19, 2018 22:38

Gozala force-pushed the ignore-negation branch 2 times, most recently from e157dc3 to aba3201 Compare July 20, 2018 19:14

Gozala force-pushed the ignore-negation branch from aba3201 to a2b366c Compare July 20, 2018 19:34

Rob--W reviewed Jul 20, 2018

View reviewed changes

test: Add test for negated ignore path

2538509

Gozala force-pushed the ignore-negation branch from a2b366c to 418ccc8 Compare July 20, 2018 20:21

Rob--W approved these changes Jul 20, 2018

View reviewed changes

Gozala changed the title ~~Allow negation in --ignore-files~~ feat: Allow negation in --ignore-files Jul 20, 2018

feat: Allow negation in --ignore-files

19d7755

- Swaps minimatch for multimatch to allow opt-in inclusion of node_modules subset - Fix test introduced in previous commit - Fix mozilla#1347

Gozala force-pushed the ignore-negation branch from 418ccc8 to 19d7755 Compare July 20, 2018 21:23

rpl requested changes Jul 21, 2018

View reviewed changes

test: Exercise build command with negated ignore

7d48421

Gozala mentioned this pull request Jul 23, 2018

Restructure demos mozilla/libdweb#27

Closed

rpl approved these changes Jul 24, 2018

View reviewed changes

rpl merged commit 065188d into mozilla:master Jul 24, 2018

Gozala mentioned this pull request Jul 26, 2018

Allow including some files from node_modules #1347

Closed

Rob--W mentioned this pull request Sep 7, 2018

ignore-files no longer works with "!(...)" glob pattern #1365

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow negation in --ignore-files #1348

feat: Allow negation in --ignore-files #1348

Gozala commented Jul 19, 2018 •

edited

coveralls commented Jul 19, 2018 •

edited

Rob--W commented Jul 20, 2018

rpl commented Jul 20, 2018

Gozala commented Jul 20, 2018

Gozala commented Jul 20, 2018

Rob--W left a comment

Rob--W Jul 20, 2018

Rob--W Jul 20, 2018

Gozala commented Jul 20, 2018

Rob--W left a comment

Gozala commented Jul 20, 2018

Gozala commented Jul 20, 2018

rpl left a comment

Gozala commented Jul 23, 2018

Gozala commented Jul 23, 2018

rpl commented Jul 24, 2018

	it('zips a package', () => {
	const zipFile = new ZipFile();

	return withTempDir(
	(tmpDir) =>
	build({
	sourceDir: fixturePath('minimal-web-ext'),
	artifactsDir: tmpDir.path(),
	})
	.then((buildResult) => {
	assert.match(buildResult.extensionPath,
	/minimal_extension-1\.0\.zip$/);
	return buildResult.extensionPath;
	})
	.then((extensionPath) => zipFile.open(extensionPath))
	.then(() => zipFile.extractFilenames())
	.then((fileNames) => {
	fileNames.sort();
	assert.deepEqual(fileNames,
	['background-script.js', 'manifest.json']);
	return zipFile.close();
	})
	);
	});

feat: Allow negation in --ignore-files #1348

feat: Allow negation in --ignore-files #1348

Conversation

Gozala commented Jul 19, 2018 • edited

coveralls commented Jul 19, 2018 • edited

Rob--W commented Jul 20, 2018

rpl commented Jul 20, 2018

Gozala commented Jul 20, 2018

Gozala commented Jul 20, 2018

Rob--W left a comment

Choose a reason for hiding this comment

Rob--W Jul 20, 2018

Choose a reason for hiding this comment

Rob--W Jul 20, 2018

Choose a reason for hiding this comment

Gozala commented Jul 20, 2018

Rob--W left a comment

Choose a reason for hiding this comment

Gozala commented Jul 20, 2018

Gozala commented Jul 20, 2018

rpl left a comment

Choose a reason for hiding this comment

Gozala commented Jul 23, 2018

Gozala commented Jul 23, 2018

rpl commented Jul 24, 2018

Gozala commented Jul 19, 2018 •

edited

coveralls commented Jul 19, 2018 •

edited