feat: Added --ignore-files option and build now ignores artifacts folder #753

eight04 · 2017-01-15T15:53:50Z

Fixes #131, Fixes #186

coveralls · 2017-01-15T15:59:15Z

Coverage remained the same at 100.0% when pulling c497465 on eight04:dev-file-filter into 7d20a3c on mozilla:master.

kumar303

Hi, thanks for digging into this issue!

It's on the right track but here are some change requests:

--ignore-files should be a global option so that other commands can benefit from it
The ignoreFiles and artifactsDir logic needs to move out of build and into FileFilter directly since both lint and run's file watcher use the FileFilter too. Both of those commands will need the same ignoreFiles and artifactsDir logic.

eight04 · 2017-01-15T23:12:49Z

Seems that there are only 2 tests using filesToIgnore option. Can we replace it completely with ignoreFiles?
https://github.com/mozilla/web-ext/search?utf8=%E2%9C%93&q=filesToIgnore

coveralls · 2017-01-16T04:08:50Z

Coverage decreased (-0.9%) to 99.14% when pulling cae516e on eight04:dev-file-filter into 7d20a3c on mozilla:master.

kumar303 · 2017-01-17T04:39:00Z

Seems that there are only 2 tests using filesToIgnore option. Can we replace it completely with ignoreFiles?

I think so. It is only used by build to set some defaults but it currently relies on minimatch patterns for nested directories.

kumar303

This is on the right track, thanks! Let me know if you get stuck.

kumar303 · 2017-01-17T04:45:10Z

src/cmd/run.js

@@ -257,14 +262,21 @@ export default async function run(
      }

      log.info('The extension will reload if any source file changes');
+      const createWatcher = (...args) => {


The coverage report says this function never gets executed by the test suite. I think you'd have two options:

Try to think of a test that calls run() that covers it

Replace createWatcher with a dependency injection parameter and write a separate unit test for it. This is the typical pattern we use elsewhere because it usually means the tests are easier to write.

I have to solve #751 to do the "run" test.

ah, yes of course! OK, let's try to land #751 first so you can merge it into this branch when it hits master.

kumar303 · 2017-01-17T04:50:10Z

src/cmd/run.js

@@ -257,14 +262,21 @@ export default async function run(
      }

      log.info('The extension will reload if any source file changes');
+      const createWatcher = (...args) => {


I'd rather not see args get mutated because who knows what babel does to it under the hood. Could it be done more directly, like:

const createWatcher = (watcherParams, ...args) => { // ... return defaultWatcherCreator({ ...watcherParams, shouldWatchFile: (file) => fileFilter.wantFile(file), }, ...args); }

kumar303 · 2017-01-17T04:53:00Z

src/cmd/run.js

+        });
+        args[0].shouldWatchFile = (file) => fileFilter.wantFile(file);
+        return defaultWatcherCreator(...args);
+      };


Is this callback necessary? I see that sourceDir and artifactsDir are already passed to reloadStrategy and eventually the watcher where it instantiates its own FileFilter anyway (in lieu of a callback). Maybe you can just pass ignoreFiles all the way through? Sorry, this code has been rewritten a couple times to try and alleviate the headache (but it still hurts).

You're right. I should make reloadStrategy accept ignoreFiles and build shouldWatchFile inside reloadStrategy.

Also maybe we should remove FileFilter from the watcher so it doesn't depend on FileFilter anymore. Make shouldWatchFile default to () => true.

Yeah, there is probably a bit too much complexity here.

kumar303 · 2017-01-17T05:02:05Z

src/program.js

@@ -246,6 +246,12 @@ Example: $0 --help run.
      describe: 'Show verbose output',
      type: 'boolean',
    },
+    'ignore-files': {
+      alias: 'i',
+      describe: 'Files to ignore.',


This should be more explicit and should show an example of ignoring multiple files. It should say that the file must be an absolute path. I think an example of multiple paths would look like this, no? --ignore-files /path/to/first.js /path/to/second.js

It can be relative path. FileFilter resolves it with sourceDir.

kumar303 · 2017-01-17T05:11:52Z

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

@@ -48,6 +53,50 @@ describe('build', () => {
    );
  });

+  it('ignore artifacts dir if included by source dir when zipping', () => {


rejoice! Since you moved all of the ignoring logic, you no longer need such a heavyweight test. Here's what you can do:

you can delete all of the fixture directories (with the manifest, etc)

you can simply assert that build is configuring the file watcher correctly with its value of ignoreFiles. Here are some examples of how we do things like that.

kumar303 · 2017-01-17T05:13:22Z

src/util/file-filter.js

@@ -0,0 +1,87 @@
+/* @flow */


Good call on moving this to its own file. You also need to move the tests out of test.build.js. The new ignoreFiles needs test coverage too.

I found some bugs when writing tests for FileFilter.

it('ignore artifactsDir and its content', () => { const filter = new FileFilter({ artifactsDir: 'artifacts', }); assert.equal(filter.wantFile('artifacts'), false); assert.equal(filter.wantFile('artifacts/some.js'), false); // assertion error });

It might be fine for build.js, since the zipDir will skip the content if the parent dir is skipped.

But it cause problem for watcher.js. Make the content inside artifactsDir becomes wanted files. This bug applies to .git and node_modules folders as well.

I'm not sure how should we fix it. Just explicitly ignore all content inside the folder?

filesToIgnore = [ '**/*.xpi', '**/*.zip', '**/.*', // any hidden file + '**/.*/**/*', // any content in the hidden folder '**/node_modules', + '**/node_modules/**/*', ],

These issues are both resolved in the patch now.

coveralls · 2017-01-17T18:28:04Z

Coverage decreased (-0.1%) to 99.858% when pulling 6e4d8ae on eight04:dev-file-filter into 7d20a3c on mozilla:master.

…util/test.file-filter.js

coveralls · 2017-01-17T19:28:36Z

Coverage decreased (-0.1%) to 99.858% when pulling 452e382 on eight04:dev-file-filter into 7d20a3c on mozilla:master.

eight04 · 2017-01-25T03:01:06Z

How do I fix this error?

BTW, what does the vertical bar mean in type definition? I can't find them in flow's document.

exports type ... = {|
	...
|};

kumar303 · 2017-01-25T16:31:28Z

How do I fix this error?

Huh, looks like a Flow bug. Flow is smart enough to see that you checked the type of the variable in an if statement but it seems to forget the if statement after one line of code. I moved the resolve() call right under the if statement and it worked. However, you can see that the second line still needs protection against undefined values (which is a Flow bug). Here's my patch:

diff --git a/src/util/file-filter.js b/src/util/file-filter.js
index 23a941d..13d046a 100644
--- a/src/util/file-filter.js
+++ b/src/util/file-filter.js
@@ -68,11 +68,13 @@ export class FileFilter {
    *  Resolve relative path to absolute path if sourceDir is setted.
    */
   resolve(file: string): string {
-    if (typeof this.sourceDir === 'string') {
+    if (this.sourceDir !== undefined) {
+      const resolvedPath = path.resolve(this.sourceDir, file);
       log.debug(
-        `Adding sourceDir ${this.sourceDir} to the beginning of file ${file}`
+        `Resolved path ${file} with sourceDir ${this.sourceDir || ''} ` +
+        `to ${resolvedPath}`
       );
-      return path.resolve(this.sourceDir, file);
+      return resolvedPath;
     }
     return normalizeResolve(file);
   }

I also made the debug statement more verbose to help detect the situation you mentioned above when the path is absolute (which is when it won't be pre-pended with sourceDir).

kumar303 · 2017-01-25T16:36:30Z

BTW, what does the vertical bar mean in type definition?

Sigh, I can't find it in the docs either. It's a very important feature. If you have a function like this:

type Params = {|
  size: 'small' | 'large',
|}

function render({size}: Params) {
}

And you call it like this:

render({size: 'small', color: 'blue'});

then Flow will raise an error because color is not a valid parameter. If you do not use the pipes when defining Params, Flow will not raise an error. I think this was a pretty major flaw in Flow's original implementation but they couldn't fix it without new syntax because of backwards compatibility. All object types should always use pipes (except there are a few open bugs).

kumar303 · 2017-01-25T16:42:28Z

src/util/file-filter.js

+ */
+export class FileFilter {
+  filesToIgnore: Array<string>;
+  sourceDir: string | typeof undefined;


I guess this works. I suggested the wrong thing originally but I was thinking of:

sourceDir: ?string;

They are pretty much the same though. The one with the question marks allows it to be null which is the only difference.

coveralls · 2017-01-25T23:57:10Z

Coverage remained the same at 100.0% when pulling 1795c0e on eight04:dev-file-filter into b558ec8 on mozilla:master.

kumar303

Thanks again for all the quick turnarounds! This looks good to me but because it touches so many commands, I'd like @rpl to also review it.

rpl

Hi @eight04
My apologies for the waiting time, this PR looks almost great, thanks for working on it!

I've added some comments related to some minor changes (most of them are small nits, the most important ones are related to the yargs configuration of the new cli option).

Let me know if any of the comment is not clear enough or if you disagree with any of the requested changes.

rpl · 2017-01-31T15:52:24Z

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

@@ -49,6 +49,27 @@ describe('build', () => {
    );
  });

+  it('configures a build', () => {


this test case description could probably be a bit more specific (e.g. it('configures a build command with the expected fileFilter', ...))

rpl · 2017-01-31T15:55:26Z

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

@@ -299,6 +300,23 @@ describe('run', () => {
      assert.typeOf(callArgs.onChange, 'function');
    });

+    it('configure fileFilter, shouldWatchFile', () => {


how about it('configures a run command with the expected fileFilter', ...))?

rpl · 2017-01-31T15:56:18Z

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

-        assert.equal(fileFilter.wantFile.called, true);
-        assert.equal(fileFilter.wantFile.firstCall.args[0], 'manifest.json');
-      });
+  it('configures and passes a fileFilter to the linter', () => {


how about it('configures a lint command with the expected fileFilter', ...)?

rpl · 2017-01-31T15:59:05Z

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

+      paths.forEach((file) => {
+        assert.equal(
+          path.resolve(src, file),
+          path.resolve(src) + path.sep + normalizeResolve(file)


can we use path.join(path.resolve(src), normalizeResolve(file)) here?

I guess it's OK? This test is to make sure:

normalizeResolve(relativePath) == stripRoot(path.resolve(root, relativePath), root)

rpl · 2017-01-31T16:10:37Z

src/program.js

@@ -246,6 +246,14 @@ Example: $0 --help run.
      describe: 'Show verbose output',
      type: 'boolean',
    },
+    'ignore-files': {


this options should require an argument (by adding the requiresArg: true to the yargs properties for the option).

It seems that setting requiresArg would force yargs to validate ignoreFiles is not a empty list. Since the ignoreFiles defaults to an empty list, this make yargs always argue Missing argument value: ignore-files.

This comment is supposed to be part of the last review comments but it has not been submitted by github, and so I added it here as a separate comment.

@eight04 yargs can be very annoying sometimes :-)

Anyway, it looks like that we can use a combination of yargs option config properties to make it work as expected:

'ignore-files': { alias: 'i', describe: '...', demand: false, requiresArg: true, type: 'array', },

Basically it seems that we have to:

omit the empty array currently set as its default value

add a demand: false config to allow the --ignore-files option to be undefined (which is going to mean "that the option has not been specified")

add the requiresArg: true which force "--ignore-files" to have a value when specified

I briefly tried the above config locally and it seems to work as expected,
let me know if it doesn't for you.

rpl · 2017-01-31T16:38:58Z

src/program.js

+      alias: 'i',
+      describe: 'A list of glob patterns to define which files should be ' +
+                'ignored. (Example: --ignore-files=path/to/first.js ' +
+                'path/to/second.js **/*.log)',


In the help message we should suggest to quote the patterns (e.g. the **/*.log), or what is going to happen in most of the shells is that "the glob pattern will be expanded up front and a long list of the resulting files will be passed to the web-ext command" (instead of the single glob entry as expected).
e.g. --ignore-files=path/to/file.js "**/*.log"

rpl · 2017-01-31T17:05:16Z

src/util/file-filter.js

+  sourceDir: ?string;
+
+  constructor({
+    filesToIgnore = [


The FileFilter now takes two parameters with two very similar names but they have different meaning, it would be nice to make it more clear by make the names a bit more descriptive (e.g. filesToIgnore renamed to defaultFilesToIgnore, and ignoreFiles renamed to customFilesToIgnore?)

If we change ignoreFiles to customFilesToIgnore, every calls to createFileFilter would also need to be changed to createFileFilter({sourceDir, artifactsDir, customFilesToIgnore: ignoreFiles}).

How about to change filesToIgnore to defaultIgnorePatterns and keep ignoreFiles so createFileFilter could be called nicely as createFileFilter({sourceDir, artifactsDir, ignoreFiles})?

@eight04 yeah, I agree that not being able to use the ES6 syntax shortcut is a pity.

What I'd like to achieve is to make it clear that the FileFilter constructor property currently named filesToIgnore is not overridden by the new ignoreFiles property, but extended.

How about rename filesToIgnore to baseIgnoredPatterns (or commonIgnoredPatterns) and leave ignoreFiles unmodified?

coveralls · 2017-02-01T02:16:40Z

Coverage remained the same at 100.0% when pulling 09bee46 on eight04:dev-file-filter into b558ec8 on mozilla:master.

rpl

Hi @eight04
Thanks for the last round of updates, This is almost ready!
Follows some additional comments on the last two pending issues (the FileFilter contructor property names and the requiresArg option on the yargs option configuration).

rpl · 2017-02-01T14:36:29Z

src/util/file-filter.js

+  sourceDir: ?string;
+
+  constructor({
+    filesToIgnore = [


@eight04 yeah, I agree that not being able to use the ES6 syntax shortcut is a pity.

What I'd like to achieve is to make it clear that the FileFilter constructor property currently named filesToIgnore is not overridden by the new ignoreFiles property, but extended.

How about rename filesToIgnore to baseIgnoredPatterns (or commonIgnoredPatterns) and leave ignoreFiles unmodified?

coveralls · 2017-02-01T15:33:24Z

Coverage remained the same at 100.0% when pulling 6e45154 on eight04:dev-file-filter into b558ec8 on mozilla:master.

coveralls · 2017-02-01T15:33:24Z

Coverage remained the same at 100.0% when pulling 6e45154 on eight04:dev-file-filter into b558ec8 on mozilla:master.

rpl

This PR looks great, thanks @eight04!

kumar303 · 2017-02-01T16:46:50Z

Thanks again for all your work on this and especially for all the quick turnarounds.

caitmuenster · 2017-02-01T17:34:34Z

Nice work! 👍

eight04 added 4 commits January 15, 2017 21:24

feat: add artifactsDir to ignoreFileList

19504b1

feat: add --ignore-files option to build command

cd70980

test: --ignore-files option, ignore artifacts dir

b48244d

fix: resolve to absolute path

c497465

muffinresearch added the state: pull request ready label Jan 15, 2017

kumar303 changed the title ~~Add --ignore-files option & ignore artifacts folder~~ feat: Added --ignore-files option and build now ignores artifacts folder Jan 15, 2017

kumar303 suggested changes Jan 15, 2017

View reviewed changes

kumar303 self-assigned this Jan 15, 2017

eight04 added 6 commits January 16, 2017 10:52

fix: make ignore-files option global

07cc1a1

refactor: split FileFilter out from build.js to util/file-filter.js

e9e583a

fix: make FileFilter accept sourceDir, ignoreFiles, artifactsDir

dd3188b

fix: let build.js build fileFilter with more options

619342d

fix: let lint.js build FileFilter with more options

0cb2a43

fix: make run.js be able to pass shouldWatchFile into watcher

cae516e

kumar303 suggested changes Jan 17, 2017

View reviewed changes

eight04 added 2 commits January 17, 2017 23:17

fix: make reloadStrategy accept ignoreFiles

30254c5

fix: better description for --ignore-files

f51841b

kumar303 mentioned this pull request Jan 17, 2017

Added support for .xpiignore (at sourceDir) with node-ignore #703

Closed

eight04 added 2 commits January 18, 2017 02:09

test: test new options: artifactsDir, sourceDir, ignoreFiles

de6ecfd

fix: normalize relative path and correctly ignore a directory

6e4d8ae

eight04 added 3 commits January 18, 2017 02:50

refactor: split filt-filter test out from test.build.js to unit/test-…

173333b

…util/test.file-filter.js

test: normalizeResolve

fc02fc2

test: rewrite ignore artifacts dir when zipping test

452e382

fix: minor fix in normalizeResolve

87f30bc

eight04 added 4 commits January 25, 2017 09:43

fix: minor fixes to sentences, typo, log message

e95b538

fix: minor fixes to file-filter

0d35950

test: drop calledWith

6f61733

fix: type check

1ad7c5b

kumar303 reviewed Jan 25, 2017

View reviewed changes

fix: type check

1795c0e

kumar303 approved these changes Jan 27, 2017

View reviewed changes

kumar303 requested a review from rpl January 27, 2017 18:14

kumar303 assigned rpl Jan 27, 2017

kumar303 mentioned this pull request Jan 28, 2017

chore(package): update request to version 2.79.0 mozilla/sign-addon#245

Merged

rpl requested changes Jan 31, 2017

View reviewed changes

eight04 added 2 commits February 1, 2017 09:33

fix: minor fixes to test/command messages

214e09e

fix: use path.join instead of path.sep

09bee46

rpl requested changes Feb 1, 2017

View reviewed changes

eight04 added 2 commits February 1, 2017 23:14

fix: use requiresArg, demand for ignore-files

70db989

fix: change FileFilter param filesToIgnore -> baseIgnoredPatterns

6e45154

rpl approved these changes Feb 1, 2017

View reviewed changes

kumar303 merged commit be62a54 into mozilla:master Feb 1, 2017

kumar303 removed the state: pull request ready label Feb 1, 2017

rpl mentioned this pull request Feb 6, 2017

feat: Added reload from the terminal by pressing a key #746

Merged

snyk-bot mentioned this pull request Jul 6, 2021

[Snyk] Upgrade sign-addon from 3.3.0 to 3.5.0 terrorizer1980/web-ext#3

Open

feat: Added --ignore-files option and build now ignores artifacts folder #753

feat: Added --ignore-files option and build now ignores artifacts folder #753

Conversation

eight04 commented Jan 15, 2017 • edited by kumar303 Loading

coveralls commented Jan 15, 2017 • edited Loading

kumar303 left a comment

Choose a reason for hiding this comment

eight04 commented Jan 15, 2017

coveralls commented Jan 16, 2017 • edited Loading

kumar303 commented Jan 17, 2017

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 17, 2017 • edited Loading

coveralls commented Jan 17, 2017 • edited Loading

eight04 commented Jan 25, 2017

kumar303 commented Jan 25, 2017

kumar303 commented Jan 25, 2017

Choose a reason for hiding this comment

coveralls commented Jan 25, 2017 • edited Loading

kumar303 left a comment

Choose a reason for hiding this comment

rpl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 1, 2017 • edited Loading

rpl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 1, 2017

coveralls commented Feb 1, 2017

rpl left a comment

Choose a reason for hiding this comment

kumar303 commented Feb 1, 2017

caitmuenster commented Feb 1, 2017

eight04 commented Jan 15, 2017 •

edited by kumar303

Loading

coveralls commented Jan 15, 2017 •

edited

Loading

coveralls commented Jan 16, 2017 •

edited

Loading

coveralls commented Jan 17, 2017 •

edited

Loading

coveralls commented Jan 17, 2017 •

edited

Loading

coveralls commented Jan 25, 2017 •

edited

Loading

coveralls commented Feb 1, 2017 •

edited

Loading