chore: Rewrote watchChange to make use of async await

[ankushduacodes]

Rewrote watchChange to make use of async-await:

web-ext/tests/unit/test.watcher.js

Lines 22 to 103 in 629aa14

	const watchChange = ({
	watchFile,
	touchedFile,
	}: AssertWatchedParams = {}) => withTempDir(
	(tmpDir) => {
	const artifactsDir = path.join(tmpDir.path(), 'web-ext-artifacts');
	const someFile = path.join(tmpDir.path(), touchedFile);
	if (watchFile) {
	watchFile = watchFile.map((f) => path.join(tmpDir.path(), f));
	}
	var resolveChange;
	const whenFilesChanged = new Promise((resolve) => {
	resolveChange = resolve;
	});
	const onChange = sinon.spy(() => {
	resolveChange();
	});
	let watchedFilePath;
	let watchedDirPath;
	return fs.writeFile(someFile, '<contents>')
	.then(() => {
	return onSourceChange({
	sourceDir: tmpDir.path(),
	watchFile,
	artifactsDir,
	onChange,
	shouldWatchFile: () => true,
	});
	})
	.then((watcher) => {
	const {fileWatchers, directoryWatchers} = watcher;
	let watchedFile;
	let watchedDir;
	if (fileWatchers?.size > 0) {
	watchedFile = Array.from(fileWatchers.values())[0];
	}
	if (directoryWatchers?.size > 0) {
	watchedDir = Array.from(directoryWatchers.values())[0];
	}
	watchedFilePath = watchedFile && watchedFile.path;
	watchedDirPath = watchedDir && watchedDir.path;
	return watcher;
	})
	.then((watcher) => {
	return fs.utimes(someFile, Date.now() / 1000, Date.now() / 1000)
	.then(() => watcher);
	}).then((watcher) => {
	const assertParams = {
	onChange,
	watchedFilePath,
	watchedDirPath,
	tmpDirPath: tmpDir.path(),
	};
	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, assertParams);
	});
	}),
	// Time out if no files are changed
	new Promise((resolve) => setTimeout(() => {
	watcher.close();
	resolve(assertParams);
	}, 500)),
	]);
	});
	}
	);

[ankushduacodes]

@rpl I am getting a flow type error. Unfortunately, I don't have much clue of whats wrong. Any insight?
I tried adding a new type with two properties something like :

type fileWatcher = {
  fileWatchers: Map<string, Watchpack>,
  directoryWatchers: Map<string, Watchpack>,
}

but now I am getting other errors saying can not destructure watcher

The unit test the running just fine as I tired setting CI_SKIP_FLOWCHECK env option to skip flow checks.

Question:
Why do we skip flow checks on windows?

[Rob--W]

Question:
Why do we skip flow checks on windows?

Running it once (on Linux) is sufficient. CI on Windows is generally slower.

web-ext/.circleci/config.yml

Lines 234 to 235 in 762a1cf

	# `flow check` does a static analysis, no need to run more than once
	# per worflow run.

[rpl]

@rpl I am getting a flow type error. Unfortunately, I don't have much clue of whats wrong. Any insight?
The unit test the running just fine as I tired setting CI_SKIP_FLOWCHECK env option to skip flow checks.

Sure thing, the issue is being triggered by this changes just because in the "chained promises" form flow wasn't able to guess what was the type of that watcher and so flow does fallback to guess its type as any.

"Flow inferring the type as any" basically means going into the "untyped domain", as if there isn't any explicit type checking (quite some time ago I did wrote a tool, flow-coverage-report, to keep an eye on how much of the code is actually typed) to actually , in this case is fine because this is a test and that code would fail if watchpack will change its internals and those property do not work anymore as they used to be or are being removed.

At the moment we are writing the types for a subset of the dependencies, and only for the parts that we are actually using and flow does care about, the one for watchpack is this one: flow-typed/watchpack.decl.js (as you can see I've been lazy and I did declare only the bare minimum, which makes sense because we do have to maintain those types over the watchpack releases).

To fix the flow failure we do have two options:

1. tell flow to ignore that type checking error in the test (we already use this kind of strategy in other tests when appropriate, and without abusing it too much)[1]:

       // $FlowIgnore: retrieve internal Watchpack properties for testing purpose.
       const {fileWatchers, directoryWatchers} = watcher;

2. Define the additional property in the flow typed declaration file (without going too much in the detail, because we shouldn't use any of those internal properties anywhere besides this test):

diff --git a/flow-typed/watchpack.decl.js b/flow-typed/watchpack.decl.js
index 5e6cf62..4a8e98d 100644
--- a/flow-typed/watchpack.decl.js
+++ b/flow-typed/watchpack.decl.js
@@ -16,6 +16,9 @@ declare module "watchpack" {
     constructor(options?: WatchpackOptions): Watchpack,
     close(): void,
     watch(options: WatchOptions): void,
+
+    fileWatchers: Map<string, {path: string}>,
+    directoryWatchers: Map<string, {path: string}>,
   }

I think that in this case we may opt for option 1, in the end if we don't add those internal properties to the flow typed declarations we are not going to be tempted to use them on the implementation side :-P

Question:
Why do we skip flow checks on windows?

Mainly because the windows workers are usually annoyingly slow and given that flow does just static analysis running the flow checks on windows isn't really needed.
Honestly I can't recall if there was also any issue in running flow in the CI windows workers in the past, but it should work just fine in a windows system and so we do only skip it on the CI.

---

[1]: You can read some more details about what are the flow suppress comments here https://flow.org/en/docs/config/options/#toc-suppress-comment-regex (the config option is now deprecated and the strings to be used are hardcoded, but the feature is still there and supported)

[ankushduacodes]

tell flow to ignore that type checking error in the test (we already use this kind of strategy in other tests when appropriate, and without abusing it too much)[1]:

     // $FlowIgnore: retrieve internal Watchpack properties for testing purpose.
     const {fileWatchers, directoryWatchers} = watcher;

Yes, I tried using // $FlowFixMe earlier and it did the trick. On it.

in this case is fine because this is a test and that code would fail if watchpack will change its internals and those property do not work anymore as they used to be or are being removed.

Makes sense.

Mainly because the windows workers are usually annoyingly slow and given that flow does just static analysis running the flow checks on windows isn't really needed.

Got it.

Thanks, @rpl and @Rob--W for the information on it.

[rpl]

lgtm, it is definitely nicer :-)

@ankushduacodes Thanks a lot for offering to pick this up, very much appreciated!!!