feat: Added option to `--watch-file` to address eager reloading #1784

jezhou · 2019-12-14T09:07:23Z

This PR adds a --watch-file parameter, which should resolve the problem of web-ext reloading changes too eagerly when using a custom builder/bundler system like Webpack. It helps with this issue by only reloading the extension when the developer touches a specific file after building their extension (this require some configuration on the developer's part).

I got confused in the implementation details since the issue describe the feature as watching a particular file, but some of the finer details seem to focus on watching a new directory.

I wasn't sure which one to implement – file or directory – so I did file because a) it provides a finer grain of control that I think solves the problem and b) the assertions in test.watcher.js were easier to write using a file.

I think this isn't too bad for a first pass, but test.watcher.js ended up being a little tricker than I thought, so definitely need some second eyes on that. I'll annotate why I thought it was tricky below.

--wip-- [skip ci] --wip-- [skip ci] fix flow errors

jezhou · 2019-12-14T09:11:18Z

tests/unit/test.watcher.js

-          return whenFilesChanged
-            .then(() => {
+          return Promise.race([
+            whenFilesChanged
+              .then(() => {
+                watcher.close();
+                // This delay seems to avoid stat errors from the watcher
+                // which can happen when the temp dir is deleted (presumably
+                // before watcher.close() has removed all listeners).
+                return new Promise((resolve) => {
+                  setTimeout(resolve, 2, onChange);
+                });
+              }),
+            // Time out if no files are changed
+            new Promise((resolve) => setTimeout(() => {
              watcher.close();
-              sinon.assert.calledOnce(onChange);
-              // This delay seems to avoid stat errors from the watcher
-              // which can happen when the temp dir is deleted (presumably
-              // before watcher.close() has removed all listeners).
-              return new Promise((resolve) => setTimeout(resolve, 2));
-            });


The way this was written before, whenFilesChanged will not resolve unless WatchPack actually detects the changes. This leads to a problem with the new feature because with the new option, if the detected file isn't modified, this will never resolve, which fails the test.

I added a "timeout" to the promise using Promise.race() – which after experimenting with seems to correctly fail the assertions if I want them to. The only bad thing about this I guess is that one test will always take 500ms (the timeout in that second promise). I don't think that's too bad, but if there's a better way to do all of this I'd love to learn 😄

coveralls · 2019-12-14T09:13:49Z

Coverage remained the same at 100.0% when pulling 813a1a4 on jezhou:jz/add-option-watch-file into 7ee3c44 on mozilla:master.

coveralls · 2019-12-14T09:13:49Z

Coverage remained the same at 100.0% when pulling afae5f9 on jezhou:jz/add-option-watch-file into 622547b on mozilla:master.

jezhou · 2019-12-14T09:28:39Z

I think the build is failing because of npm audit. Not sure if I should fix the vulnerability in this PR or leave it to another PR?

Rob--W · 2019-12-14T11:06:24Z

I think the build is failing because of npm audit. Not sure if I should fix the vulnerability in this PR or leave it to another PR?

The error is unrelated to your change, so please create a new PR (or open a new issue).

Rob--W · 2019-12-14T11:09:37Z

src/watcher.js

  watcher.watch([], [sourceDir], Date.now());

+  if (watchFile) {


To avoid reloading too often, when watchFile is specified, sourceDir should not be watched any more. Could you update the implementation and tests with this expectation?

Even if watcher.watch ends up ignoring the previous call, the readability of the code would be better if you move the previous wacher.watch call into an } else { block.

Or something like this:

let watchedDirs = []; let watchedFiles = []; if (watchFile) { watchedFiles.push(watchFile); } else { watchedDirs.push(sourceDir); } watcher.watch(watchedFiles, watchedDirs, Date.now();

…file

jezhou · 2019-12-15T01:09:45Z

@Rob--W I added a few more assertions to make sure the Watchers in WatchPack were only watching either the watchFile or the sourceDir.

Implementation wise, I'm not sure if I need to add anything else. It looks like the WatchPack README says that calling watch() twice will override existing files and directories. It looks like that's the case when I look at the source too.

…-file

Rob--W

(forgot to submit those comments when I wrote them; apologies for the delay)

Rob--W · 2019-12-19T07:54:20Z

src/watcher.js

  watcher.watch([], [sourceDir], Date.now());

+  if (watchFile) {


Even if watcher.watch ends up ignoring the previous call, the readability of the code would be better if you move the previous wacher.watch call into an } else { block.

Or something like this:

let watchedDirs = []; let watchedFiles = []; if (watchFile) { watchedFiles.push(watchFile); } else { watchedDirs.push(sourceDir); } watcher.watch(watchedFiles, watchedDirs, Date.now();

Rob--W · 2019-12-19T08:01:38Z

tests/functional/test.cli.run.js

     () => withTempAddonDir(
       {addonPath: minimalAddonPath},
       (srcDir) => {
         const argv = [
           'run', '--verbose', '--no-reload',
           '--source-dir', srcDir,
+           '--watch-file', srcDir,


--watch-file is expected to be a file. I would expect an error if a directory is passed.

Done – I threw a UsageError here if a directory is passed in. Let me know if there's a better error to throw.

…-file

Rob--W · 2020-01-03T15:48:22Z

tests/functional/test.cli.run.js

+      return execWebExt(argv, {}).waitForExit.then(({exitCode, stdout}) => {
+        assert.notEqual(exitCode, 0);
+        // eslint-disable-next-line no-console
+        console.log(stdout);


Remove these two lines of code. You probably added them for debugging only.

Rob--W · 2020-01-03T16:02:07Z

src/cmd/run.js

+    fs.existsSync(watchFile) &&
+    fs.lstatSync(watchFile).isDirectory()
+  ) {
+    throw new UsageError(`The directory "${watchFile}" cannot be ` +


This check is not strictly necessary, but it will probably help against common mistakes.

Let's move this whole check to the src/watcher.js, right before watchedFiles.push(watchFile);.
And use isFile() instead of isDirectory. That would also defend against cases where the file path refers to non-files that are not directories, such as symlinks.

…-file

…e resolves

Rob--W

One small comment, and then it looks good to me.

Rob--W · 2020-01-04T00:16:23Z

src/watcher.js

+
+  if (watchFile) {
+    if (fs.existsSync(watchFile) && !fs.lstatSync(watchFile).isFile()) {
+      throw new UsageError(`"${watchFile}" cannot be passed` +


"cannot be passed" is vague. Could you rephrase the error message so that it is more actionable? E.g. to:

throw new UsageError(`"Invalid --watch-file value: ${watchFile}" is not a file`)

jezhou · 2020-01-05T08:00:04Z

@Rob--W do you think you can land this? I don't have write permissions for this repo 🙂

Rob--W · 2020-01-05T15:40:55Z

Thanks, merged!

Jesse Zhou added 6 commits December 12, 2019 01:11

refactor: Used object param for WatchPack instead of positional params

40a593b

feat: Added option to watch a specific directory or file

30fbd66

--wip-- [skip ci] --wip-- [skip ci] fix flow errors

test: Tested new watchFile option in watcher.js

95f4c64

test: Tested watchFile param passing in extension-runner

98defef

test: Tested the web-ext run command

02faac5

test: Tested program.js

813a1a4

jezhou commented Dec 14, 2019

View reviewed changes

jezhou changed the title ~~feat: Watch file~~ feat: Add option to --watch-file to address eager reloading Dec 14, 2019

jezhou changed the title ~~feat: Add option to --watch-file to address eager reloading~~ feat: Added option to --watch-file to address eager reloading Dec 14, 2019

jezhou marked this pull request as ready for review December 14, 2019 09:28

Rob--W reviewed Dec 14, 2019

View reviewed changes

test: Asserted that WatchPack is watching the specified directory or …

db080fe

…file

jezhou force-pushed the jz/add-option-watch-file branch from 42eb6a6 to db080fe Compare December 15, 2019 00:56

jezhou requested a review from Rob--W December 16, 2019 17:23

Merge remote-tracking branch 'origin/master' into jz/add-option-watch…

13b3b65

…-file

Rob--W reviewed Jan 2, 2020

View reviewed changes

Jesse Zhou added 6 commits January 2, 2020 17:49

Merge remote-tracking branch 'origin/master' into jz/add-option-watch…

0cfd757

…-file

style: Improve readability of file watcher functionality

7ac0c2d

fix: do not accept directories for --watch-file command

f3b8cad

fix: remove unused 'tempdir'

8555c58

test: Tested dir error when passed into --watch-file

e05221c

test: Added non-functional test for dir error in --watch-file

812e8c7

jezhou requested a review from Rob--W January 3, 2020 06:25

Rob--W reviewed Jan 3, 2020

View reviewed changes

style: removed console.log debugging statement

70a58a6

Jesse Zhou added 4 commits January 3, 2020 14:32

refactor: moved file check into the 'watcher' instead of 'run'

95b6e6a

Merge remote-tracking branch 'origin/master' into jz/add-option-watch…

439949f

…-file

style: Clean up console logs

b3553df

fix: Added return to --watch-file test in watcher to make sure promis…

f3157fc

…e resolves

jezhou requested a review from Rob--W January 4, 2020 00:10

Rob--W reviewed Jan 4, 2020

View reviewed changes

style: Rephrased the error message for --watch-file option

afae5f9

jezhou force-pushed the jz/add-option-watch-file branch from 7d72399 to afae5f9 Compare January 4, 2020 07:17

jezhou requested a review from Rob--W January 4, 2020 07:26

Rob--W approved these changes Jan 5, 2020

View reviewed changes

Rob--W merged commit 1478d6b into mozilla:master Jan 5, 2020

jezhou deleted the jz/add-option-watch-file branch January 5, 2020 15:58

snyk-bot mentioned this pull request Apr 26, 2020

[Snyk] Upgrade web-ext from 4.1.0 to 4.2.0 freaktechnik/advanced-github-notifier#151

Merged

Rob--W mentioned this pull request Sep 17, 2020

customize file watch path spec to get rid of warnings from chokidar #2022

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Added option to `--watch-file` to address eager reloading #1784

feat: Added option to `--watch-file` to address eager reloading #1784

jezhou commented Dec 14, 2019 •

edited

Loading

jezhou Dec 14, 2019

coveralls commented Dec 14, 2019

coveralls commented Dec 14, 2019 •

edited

Loading

jezhou commented Dec 14, 2019

Rob--W commented Dec 14, 2019

Rob--W Dec 14, 2019

Rob--W Dec 19, 2019

jezhou commented Dec 15, 2019

Rob--W left a comment

Rob--W Dec 19, 2019

Rob--W Dec 19, 2019

jezhou Jan 3, 2020

Rob--W Jan 3, 2020

Rob--W Jan 3, 2020

Rob--W left a comment

Rob--W Jan 4, 2020

jezhou commented Jan 5, 2020

Rob--W commented Jan 5, 2020

feat: Added option to --watch-file to address eager reloading #1784

feat: Added option to --watch-file to address eager reloading #1784

Conversation

jezhou commented Dec 14, 2019 • edited Loading

Choose a reason for hiding this comment

coveralls commented Dec 14, 2019

coveralls commented Dec 14, 2019 • edited Loading

jezhou commented Dec 14, 2019

Rob--W commented Dec 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jezhou commented Dec 15, 2019

Rob--W 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

Rob--W left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jezhou commented Jan 5, 2020

Rob--W commented Jan 5, 2020

feat: Added option to `--watch-file` to address eager reloading #1784

feat: Added option to `--watch-file` to address eager reloading #1784

jezhou commented Dec 14, 2019 •

edited

Loading

coveralls commented Dec 14, 2019 •

edited

Loading