customize file watch path spec to get rid of warnings from chokidar #2022
Comments
|
Does the It could easily be changed to support multiple files and/or directories, by Lines 596 to 602 in 11faea6 and using the multiple values at Lines 54 to 70 in 11faea6 |
|
Yes, that would work if I could specify a directory with exclusions. I want to specify the main directory, but not the node_modules folder. Where is this feature documented? I couldn't find it anywhere. Perhaps it's too new. |
|
It's currently only listed when you run It should also be documented at the Extension workshop, whose source code is at https://github.com/mozilla/extension-workshop/blob/master/src/content/documentation/develop/web-ext-command-reference.md (permalink to current version). I suppose that we can fix this bug first and then document the behavior over there. |
|
it looks like this code is relevant (from However, I was unable to easily figure out how you guys handle globs, so I'm going to need guidance before venturing a PR. Also, we'd have to change |
|
Good question! To watch files, we are using The existing glob implementation is implemented at https://github.com/mozilla/web-ext/blob/5.1.0/src/util/file-filter.js and relies on the watchpack currently takes a list of files/directories upfront, whereas file-filter / multimatch uses functions to lazily filter file/directory names. There are about three ways to implement globs:
I prefer option 2. For that the best way forwards is to ask watchpack whether they are open to adding an option to take a function to decide whether or not to watch a file, e.g. by passing a function to the |
|
@Rob--W, Thanks for the guidance! I hope to have time for a PR sometime over the next few weeks. |
|
@Rob--W I would like to take on this issue. Please let me know if I may. |
|
@Rob--W and @ankushduacodes, Sorry I haven't ventured a PR yet! I got caught up in other projects. If @ankushduacodes wants to take a shot at it, that's fine by me. |
|
@tgiardina If you have already started then it's fine, You can work on the issue. if not then I can take over. Please let me know |
|
@ankushduacodes I have not, so it's all you. |
|
@Rob--W, I am new to open-source, I would really appreciate if you can give me some pointers related to this issue and how to solve it. |
Hi @ankushduacodes, @Rob--W is currently away and so I'll take over the mentoring on this "contrib: welcome" bug. Follows some details that should help you to get started on this issue, let me know if it does help or if there is something that is still unclear and you would like to clear out before starting to work on this. Setup dev environmentIf this is the first web-ext bug you are looking into, I suggest to start by read our CONTRIBUTING.md file here, then clone the repo and try to run the existing tests (as described in the contributing guide) to make sure your development environment is correctly configured. Understand the issue (and possibly be able to reproduce)A common initial step when starting to work on an issue is to be sure to understand the issue (and possibly be able to reproduce the issue locally). Plan a fixFrom a quick read to the comments above (in particular comments #2022 (comment), #2022 (comment) and #2022 (comment)) and after taking a look to the watchpack's README here I think that the simpler and nicer way to fix this issue may be to:
The relevant part of the web-ext source code is linked in Rob's comment #2022 (comment) Cover the change with new testsEvery change should be covered by tests, to make sure that it works as expected in the scenario we care about, and also to make sure that the fix doesn't regress when we are making other (potentially totally unrelated) changes in the future. For this particular change, I think that we may want to add a unit test in tests/unit/test.watcher.js. Taking a look to the other unit test defined in the same test file (as well as other unit tests in the repository) is a good way to get a picture of how to write the new unit test (but also feel free to ping us if you need help to figure out some issue you may get stuck on). Reproducing the issueFor this particular bug, it looks that to be able to reproduce the issue we could:
The error that is mentioned in the issue description is likely system dependent (the ENOSPC is definitely the one that linux would produce in those condition, the error code and actualy limit on macOS and windows may likely differ) |
|
@rpl I will be working on this one. As you said in this comment that this issue is related to linux environment, so I have set up an Ubuntu virtual machine. But I also have mac and windows envs side loaded and will be testing those out for this issue as well. |
|
@rpl I was quiet busy for last few days as I had my mid term exams and didnt get much time to look into it further, I will actively working on this issue from today onwards, Thank you for your patience 😊 |
|
@rpl, I was trying to run if (fs.existsSync(watchFile) && !fs.lstatSync(watchFile).isFile()) {
throw new UsageError('Invalid --watch-file value: ' +
`"${watchFile}" is not a file.`);
}what is supposed to accomplish, One would think of throwing an error if the file path specified by the user using command line argument Whenever I use |
@ankushduacodes I expect that error to be triggered if you pass a path to a non file (e.g. a directory or a unix socket file etc.) as the --watch-file value. As an example, let's say that ./src is a directory then I expect that I just tried it locally and it did fail with that error for me, as I was expecting. Let me know if these additional details did help and/or if you still need help to be able to reproduce the issue locally. |
|
@rpl, I was able to reproduce the issue with no complications. I could be doing something wrong here.. (PS: I know the explanation of |
|
@rpl, Do you want me to create a new instance watchpack for ignored files? or just integrate it with the existing watcher? |
It should be configured on the Watchpack instance created in the |
My guess is that there was no directory (or other "non-file" directory entry) named e.g. Try this Given that In general when you have doubts about what is going on, it is a good idea to add some additional temporary logging in your local clone and trigger the behavior again to be able to inspect the value of some of the pieces that you have doubts about (e.g. you may have added a temporary call to console.log right before that if and do something like |
|
Understood Thank you!... So Is it an intended design that if watch-file gets a directory which does not exist, then it just ignores that provided directory is invalid? |
|
@rpl, I have made a pull request, although I have not written tests yet, Please check it out so I know if I am on the right path or do I need to change something. |
@ankushduacodes 👍 based on a very quick look to #2077 it seems that you are on the right track (I haven't pull the changes and tried them locally, but I looked to the diff and the changes seems to be propagating the new option to the Let's proceed to the next step: adding some new unit tests |
|
@rpl I was thinking of writing a test that looks something like the following code: describe('--watch-ignored is passed in', () => {
it('does not change if ignored file is touched', async () => {
const {
onChange,
} = await watchChange({
touchedFile: 'foo.txt',
watchIgnored: Array('foo.txt', 'bar.txt'),
});
sinon.assert.notCalled(onChange);
});
});and const watchChange = ({
watchFile,
touchedFile,
watchIgnored,
}: AssertWatchedParams = {}) => withTempDir(
(tmpDir) => {
const artifactsDir = path.join(tmpDir.path(), 'web-ext-artifacts');
const someFile = path.join(tmpDir.path(), touchedFile);
if (watchFile) {
watchFile = path.join(tmpDir.path(), watchFile);
}
var resolveChange;
const whenFilesChanged = new Promise((resolve) => {
resolveChange = resolve;
});
const onChange = sinon.spy(() => {
resolveChange();
});
let watchedFilePath;
let watchedDirPath;
let ignoredFilePaths;
if (watchIgnored) {
watchIgnored.forEach((file, index, arr) => {
arr[index] = path.join(tmpDir.path(), file);
});
}
return fs.writeFile(someFile, '<contents>')
.then(() => {
return onSourceChange({
sourceDir: tmpDir.path(),
watchFile,
watchIgnored,
artifactsDir,
onChange,
shouldWatchFile: () => true,
});
})
.then((watcher) => {
const watchedFile = watcher.fileWatchers[0];
const watchedDir = watcher.dirWatchers[0];
ignoredFilePaths = watchIgnored;
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(),
ignoredFilePaths,
};
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)),
]);
});
}But the problem is the assertion is failing because the onChange function is being called once but this shouldn't be happening. Here's what I have tried so far:
2: Ran
3: Ran
4: Ran
Here is the watcher that is being returned with I pass in EventEmitter {
_events: [Object: null prototype] { change: [Function (anonymous)] },
_eventsCount: 1,
_maxListeners: undefined,
options: {
ignored: [
'/Users/ankushdua/Documents/GitHub/web-ext/borderify/manifest.json',
'/Users/ankushdua/Documents/GitHub/web-ext/borderify/README.md'
],
aggregateTimeout: 200
},
watcherOptions: {
ignored: [
'/Users/ankushdua/Documents/GitHub/web-ext/borderify/manifest.json',
'/Users/ankushdua/Documents/GitHub/web-ext/borderify/README.md'
],
poll: undefined
},
fileWatchers: [],
dirWatchers: [
Watcher {
_events: [Object: null prototype],
_eventsCount: 2,
_maxListeners: undefined,
directoryWatcher: [DirectoryWatcher],
path: '/Users/ankushdua/Documents/GitHub/web-ext/borderify',
startTime: 1606979783950,
data: 0,
[Symbol(kCapture)]: false
}
],
mtimes: [Object: null prototype] {},
paused: false,
aggregatedChanges: [],
aggregatedRemovals: [],
aggregateTimeout: 0,
_onTimeout: [Function: bound _onTimeout],
[Symbol(kCapture)]: false
}Here is the watcher that is being returned with I DON'T pass in EventEmitter {
_events: [Object: null prototype] { change: [Function (anonymous)] },
_eventsCount: 1,
_maxListeners: undefined,
options: { aggregateTimeout: 200 },
watcherOptions: { ignored: undefined, poll: undefined },
fileWatchers: [
Watcher {
_events: [Object: null prototype],
_eventsCount: 3,
_maxListeners: undefined,
directoryWatcher: [DirectoryWatcher],
path: '/Users/ankushdua/Documents/GitHub/web-ext/borderify/manifest.json',
startTime: 1606979053531,
data: 0,
[Symbol(kCapture)]: false
}
],
dirWatchers: [],
mtimes: [Object: null prototype] {},
paused: false,
aggregatedChanges: [],
aggregatedRemovals: [],
aggregateTimeout: 0,
_onTimeout: [Function: bound _onTimeout],
[Symbol(kCapture)]: false
}(PS: All of the above mentioned testing was done in both macos and linux) |
|
@ankushduacodes the issue you are facing while trying to write the unit test is likely the fact that the watchpack version we are using is still emitting onChange events for the ignored files as part of an "initial scan" (see https://github.com/webpack/watchpack/blob/f7ef8e0a5266f78242abe21faa34c5d56eecd21f/lib/DirectoryWatcher.js#L289). To keep the test simple I think we can avoid to use (and change) that To make it even simpler to write the test and get it to better cover the scenario we are testing, I think we could also add a The resulting test case would look more or less like this: describe('--watch-ignored is passed in', () => {
it('does not change if ignored file is touched', () =>
withTempDir(async (tmpDir) => {
const debounceTime = 10;
const onChange = sinon.spy();
const tmpPath = tmpDir.path();
const files = ['foo.txt', 'bar.txt', 'foobar.txt'].map(
(filePath) => path.join(tmpPath, filePath)
);
const watcher = onSourceChange({
sourceDir: tmpPath,
artifactsDir: path.join(tmpPath, 'web-ext-artifacts'),
onChange,
watchIgnored: ['foo.txt'],
shouldWatchFile: () => true,
debounceTime,
});
async function waitDebounce() {
await new Promise((resolve) => setTimeout(resolve, debounceTime * 2));
}
// Verify foo.txt is being ignored.
await fs.writeFile(files[0], '<content>');
sinon.assert.notCalled(onChange);
// Verify that the other two files are not be ignored.
await fs.writeFile(files[1], '<content>');
await waitDebounce();
sinon.assert.calledOnce(onChange);
await fs.writeFile(files[2], '<content>');
await waitDebounce();
sinon.assert.calledTwice(onChange);
watcher.close();
// Leave watcher.close some time to complete its cleanup before withTempDir will remove the
// test directory.
await waitDebounce();
}));
});Let me know if this alternative test strategies does work as it should (you'll need to also make the needed changes to src/watcher.js to add the debounceTime parameter, which should default to the current debounce time hardcoded in the method). As an additional side note:
|
Ouch, thanks for the ping, apparently I missed to notice the github notification when you updated the PR last week, sorry about that. |
|
Good work guys, thanks a million <3 |
|
Opened a new issue 871 with extension-workshop repo to add docs for this feature. |
Is this a feature request or a bug?
Bug/feature? Both? A feature to avoid the bug
What is the current behavior?
running
web-ext runtriggers too many watchers and it throws errors:What is the expected or desired behavior?
I would like to customize which files are watched so it doesn't output a long list of warnings for every file in the node_modules folder.
Version information (for bug reports)
v12.18.4
6.14.6
5.0.0
The text was updated successfully, but these errors were encountered: