feat: enabled watching multiple files via --watch-file

[Dexterp37]

Fixes #2104

This allows passing multiple paths to --watch-file in order for web-ext. It tweaks the current tests to address the changes and make sure they still pass.

[codecov-io]

Codecov Report

Merging #2125 (949e9fa) into master (9e23a4c) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #2125   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           32        32           
  Lines         1567      1568    +1     
=========================================
+ Hits          1567      1568    +1     

Impacted Files	Coverage Δ
src/cmd/run.js	100.00% <ø> (ø)
src/extension-runners/index.js	100.00% <ø> (ø)
src/program.js	100.00% <ø> (ø)
src/watcher.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e23a4c...949e9fa. Read the comment docs.

[rpl]

@Dexterp37 my apologies, I really wanted to get to this sooner.

Anyway, this PR looks pretty great to me, I do have only one small request for changes (the small nit described below), would you mind to also remove the draft state from it after your next update on it?

I'm going to also give it a try locally, just in case I may notice something more that I missed by just looking to the diff in this PR.

(let me know if you are too busy to come back to update this PR, I'm ok to make that change and then land this for you).

[rpl]

@Dexterp37 would you mind to rename const path into file or filePath (or something similar and still short enough)?

I think that it is pretty common in nodejs apps and scripts to use path as the name for the result of importing the 'path' nodejs module, and so I would prefer to avoid using it for the string of the file path.

[rpl]

Note to self (and for @Dexterp37 because this may require some more changes):

• by changing the expected type of the watchFile option, there is a chance that we may need to also consider what will happen if a user do have a web-ext config file where that option is defined as a string
• if we will have to handle the potential issue describe above, then we may either opt to auto convert the option from string to array while loading the file or make sure we raise an error that makes it clear for the user what needs to be changed to fix it

[rpl]

@Dexterp37 I did a final review pass on this, and double-checked those other two corner cases I did mention in my previous comments:

• behavior on invalid type of the watchFile option when it is coming from a web-ext config file
• without any further changes, we would show to the user the following error message (which seems pretty understandable as it is, we can re-evaluate if we want to turn it into a warning and auto-convert the option value from src/config.jg as a followup before releasing the next major release, but in my opinion isn't a blocking issue for this PR):

  UsageError: The config file at /home/rpl/mozlab/projects/webext-tools/web-ext/test-config.js specified the type of "watchFile" incorrectly as "string" (expected type "array")

• behavior when watchFile option value is coming from the WEB_EXT_WATCH_FILE environment variable:
  • yargs does already convert automatically WEB_EXT_WATCH_FILE="a-file.txt" into watchFile = ["a-file.txt"], it wouldn't support passing more than one but that is an upstream yargs issue and it wouldn't impact backward compatibility at all.

If you don't mind, I would like to cover the third corner case that is described in the inline review comment below, after that we can proceed to merge this to the main branch.

[rpl]

While looking into some of the corner cases that may have not been covered yet, I did notice another detail that may be worth to cover explicitly:

If web-ext is imported as a library in a nodejs script, the caller may be passing watchFile as a string and no runtime check would prevent that to reach this part with a string value (assuming that the developer using web-ext isn't using flow and so it wouldn't be caught by the flow type checks).

In that case I think that no error would be detected because of the unexpected watchFile type, but errors will likely be raised because we would be trying to watch files named as every single letter of the watchFile string.

I think that it may be reasonable to do an explicit runtime check in src/cmd/run.js and throw an explicit error, which will also make us notice that there is a test case in "test.run.js" that was still passing the watchFile option as a string.

Here are the changes that I did apply locally to cover this corner case, would you mind to merge them into your PR?

diff --git a/src/cmd/run.js b/src/cmd/run.js
index 6a820dc..5334554 100644
--- a/src/cmd/run.js
+++ b/src/cmd/run.js
@@ -119,6 +119,11 @@ export default async function run(
     noReload = true;
   }
 
+  if (watchFile != null && (!Array.isArray(watchFile) ||
+      !watchFile.every((el) => typeof el === 'string'))) {
+    throw new UsageError('Unexpected watchFile type');
+  }
+
   // Create an alias for --pref since it has been transformed into an
   // object containing one or more preferences.
   const customPrefs = pref;
diff --git a/tests/unit/test-cmd/test.run.js b/tests/unit/test-cmd/test.run.js
index f0648a1..5171025 100644
--- a/tests/unit/test-cmd/test.run.js
+++ b/tests/unit/test-cmd/test.run.js
@@ -161,12 +161,22 @@ describe('run', () => {
     }, {firefoxApp, firefoxClient});
   });
 
+  it('throws if watchFile is not an array', async () => {
+    const cmd = prepareRun();
+    await assert.isRejected(
+      cmd.run({noReload: false, watchFile: 'invalid-value.txt' }),
+      /Unexpected watchFile type/
+    );
+  });
+
   it('can watch and reload the extension', async () => {
     const cmd = prepareRun();
     const {sourceDir, artifactsDir} = cmd.argv;
     const {reloadStrategy} = cmd.options;
 
-    const watchFile = fixturePath('minimal-web-ext', 'manifest.json');
+    const watchFile = [
+      fixturePath('minimal-web-ext', 'manifest.json'),
+    ];
 
     await cmd.run({noReload: false, watchFile });
     assert.equal(reloadStrategy.called, true);

[Dexterp37]

Thanks! I added this code to the PR:)

[rpl]

Thanks a lot @Dexterp37, this looks ready and I'm going to merge it to the main branch asap.