Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Feb 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/cmd/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export type CmdRunParams = {|
noReload: boolean,
preInstall: boolean,
sourceDir: string,
watchFile?: string,
watchFile?: Array<string>,
watchIgnored?: Array<string>,
startUrl?: Array<string>,
target?: Array<string>,
Expand Down Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions src/extension-runners/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ export class MultiExtensionRunner {
export type WatcherCreatorParams = {|
reloadExtension: (string) => void,
sourceDir: string,
watchFile?: string,
watchFile?: Array<string>,
watchIgnored?: Array<string>,
artifactsDir: string,
onSourceChange?: OnSourceChangeFn,
Expand Down Expand Up @@ -276,7 +276,7 @@ export function defaultWatcherCreator(
export type ReloadStrategyParams = {|
extensionRunner: IExtensionRunner,
sourceDir: string,
watchFile?: string,
watchFile?: Array<string>,
watchIgnored?: Array<string>,
artifactsDir: string,
ignoreFiles?: Array<string>,
Expand Down
2 changes: 1 addition & 1 deletion src/program.js
Original file line number Diff line number Diff line change
Expand Up @@ -619,7 +619,7 @@ Example: $0 --help run.
' file changes. This is useful if you use a custom' +
' build process for your extension',
demandOption: false,
type: 'string',
type: 'array',
},
'watch-ignored': {
describe: 'Paths and globs patterns that should not be ' +
Expand Down
18 changes: 11 additions & 7 deletions src/watcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export type OnChangeFn = () => any;

export type OnSourceChangeParams = {|
sourceDir: string,
watchFile?: string,
watchFile?: Array<string>,
watchIgnored?: Array<string>,
artifactsDir: string,
onChange: OnChangeFn,
Expand Down Expand Up @@ -57,18 +57,22 @@ export default function onSourceChange(
proxyFileChanges({artifactsDir, onChange, filePath, shouldWatchFile});
});

log.debug(`Watching for file changes in ${watchFile || sourceDir}`);
log.debug(
`Watching ${watchFile ? watchFile.join(',') : sourceDir} for changes`
);

const watchedDirs = [];
const watchedFiles = [];

if (watchFile) {
if (fs.existsSync(watchFile) && !fs.lstatSync(watchFile).isFile()) {
throw new UsageError('Invalid --watch-file value: ' +
`"${watchFile}" is not a file.`);
}
for (const filePath of watchFile) {
if (fs.existsSync(filePath) && !fs.lstatSync(filePath).isFile()) {
throw new UsageError('Invalid --watch-file value: ' +
`"${filePath}" is not a file.`);
}

watchedFiles.push(watchFile);
watchedFiles.push(filePath);
}
Comment on lines +68 to +75
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I added this code to the PR:)

} else {
watchedDirs.push(sourceDir);
}
Expand Down
12 changes: 11 additions & 1 deletion tests/unit/test-cmd/test.run.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
16 changes: 12 additions & 4 deletions tests/unit/test.program.js
Original file line number Diff line number Diff line change
Expand Up @@ -543,22 +543,30 @@ describe('program.main', () => {
});
});

it('calls run with a watched file', () => {
const watchFile = 'path/to/fake/file.txt';

async function testWatchFileOption(watchFile) {
const fakeCommands = fake(commands, {
run: () => Promise.resolve(),
});

return execProgram(
['run', '--watch-file', watchFile],
['run', '--watch-file', ...watchFile],
{commands: fakeCommands})
.then(() => {
sinon.assert.calledWithMatch(
fakeCommands.run,
{watchFile}
);
});
}

it('calls run with a watched file', () => {
testWatchFileOption(['path/to/fake/file.txt']);
});

it('calls run with multiple watched files', () => {
testWatchFileOption(
['path/to/fake/file.txt', 'path/to/fake/file2.txt']
);
});

async function testWatchIgnoredOption(watchIgnored) {
Expand Down
10 changes: 5 additions & 5 deletions tests/unit/test.watcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {withTempDir} from '../../src/util/temp-dir';
import { makeSureItFails } from './helpers';

type AssertWatchedParams = {
watchFile?: string,
watchFile?: Array<string>,
touchedFile: string,
}

Expand All @@ -28,7 +28,7 @@ describe('watcher', () => {
const someFile = path.join(tmpDir.path(), touchedFile);

if (watchFile) {
watchFile = path.join(tmpDir.path(), watchFile);
watchFile = watchFile.map((f) => path.join(tmpDir.path(), f));
}

var resolveChange;
Expand Down Expand Up @@ -116,7 +116,7 @@ describe('watcher', () => {
watchedDirPath,
tmpDirPath,
} = await watchChange({
watchFile: 'foo.txt',
watchFile: ['foo.txt'],
touchedFile: 'foo.txt',
});

Expand All @@ -132,7 +132,7 @@ describe('watcher', () => {
watchedDirPath,
tmpDirPath,
} = await watchChange({
watchFile: 'bar.txt',
watchFile: ['bar.txt'],
touchedFile: 'foo.txt',
});

Expand All @@ -143,7 +143,7 @@ describe('watcher', () => {

it('throws error if a non-file is passed into --watch-file', () => {
return watchChange({
watchFile: '/',
watchFile: ['/'],
touchedFile: 'foo.txt',
}).then(makeSureItFails()).catch((error) => {
assert.match(
Expand Down