watch: fix reloading for rebuild/compiled files #45259
Conversation
When watching folders that contain files that result from a build step, e.g: TypeScript output, stored template result, etc. The watcher will not reload the application in case one of these resulting files gets regenerated, given that when these files are removed they're no longer tracked by the watcher. This changeset enables reloading the app when changing files that result from a build step by removing the reset of the tracked filtered files on every reload. Fixing the ability to watch and reload the app when a previously-known output file is changed. Signed-off-by: Ruy Adorno <ruyadorno@google.com>
ruyadorno
force-pushed
the
fix-watch-reload-rebuilt-files
branch
from
15b48f6
to
df6b0cd
Compare
| child.kill(); | ||
| await once(child, 'exit'); | ||
|
|
||
| // TODO(ruyadorno): fs.watch is flaky when removing files from a watched |
There was a problem hiding this comment.
Can you create an issue for this and assign it to me? And can you give platform information for this line, since fs.watch has different implementations on different OS
| await once(child.stdout, 'data'); | ||
| rmSync(dependency, { force: true }); | ||
|
|
||
| await setTimeout(600); // throttle + 100 |
There was a problem hiding this comment.
Can you use common.platformTimeout(600) for all values for setTimeout?
| // This should be an assertion for the failure instead of an if block once | ||
| // the source of this flakyness gets resolved. | ||
| if (stdout.match(/Failed/g)?.length) { | ||
| assert.strictEqual(stdout.split('Failed')[1].match(/Restarting/g)?.length, 1, new Error('should have restarted after fail')); |
There was a problem hiding this comment.
Can you also add an assert for the length of split('Failed') operation?
| it('should watch changes to previously loaded dependencies', async () => { | ||
| const dependencyContent = 'module.exports = {}'; | ||
| const dependency = createTmpFile(dependencyContent); | ||
| const relativeDependencyPath = `./${path.basename(dependency)}`; |
There was a problem hiding this comment.
does path.relative(path.basename(dependency)) produces the same result for this line? if yes, can we use it?
| const child = spawn(execPath, ['--watch', '--no-warnings', dependant], { encoding: 'utf8' }); | ||
| child.stdout.on('data', (data) => { stdout += data; }); | ||
| child.stderr.on('data', (data) => { stderr += data; }); | ||
| child.on('error', (err) => { throw err; }); |
There was a problem hiding this comment.
| child.on('error', (err) => { throw err; }); | |
| child.on('error', common.mustNotCall()); |
| if (stdout.match(/Failed/g)?.length) { | ||
| assert.strictEqual(stdout.split('Failed')[1].match(/Restarting/g)?.length, 1, new Error('should have restarted after fail')); | ||
| assert.ok(stderr.match(/MODULE_NOT_FOUND/g)?.length, new Error('should have failed for not finding the removed file')); |
There was a problem hiding this comment.
| if (stdout.match(/Failed/g)?.length) { | |
| assert.strictEqual(stdout.split('Failed')[1].match(/Restarting/g)?.length, 1, new Error('should have restarted after fail')); | |
| assert.ok(stderr.match(/MODULE_NOT_FOUND/g)?.length, new Error('should have failed for not finding the removed file')); | |
| if (stdout.includes('Failed')) { | |
| assert.ok(stdout.split('Failed')[1].includes('Restarting'), new Error('should have restarted after fail')); | |
| assert.ok(stderr.includes('MODULE_NOT_FOUND'), new Error('should have failed for not finding the removed file')); |
We should use %String.prototype.includes% here instead because we're only checking if those values are included, although for the first assert we can keep it as is if we really need to check that Restarting only appears once.
There was a problem hiding this comment.
Or assert.match, to get a helpful string comparison between what was expected in case the test fails.
There was a problem hiding this comment.
this change can lead to undesired (or at least debatable) behaviors,
for example when running
const storage = require('./in-memory');and then changing to
const storage = require('./hdd');I would expect changes to ./in-memory to be ignored since they are not part of the dependency tree.
as I mentioned this expectation of mine is debatable, but I think the real issue this PR should face (and that could simplify tests), is to watch modules that do not exist yet but that are required.
meaning a better implementation would be to send a watch:require / watch:import message over IPC whenever we throw MODULE_NOT_FOUND or ERR_MODULE_NOT_FOUND, respectively, WDYT?
|
I am not blocking this PR since already blocked |
ruyadorno
changed the title
|
@MoLow that's a great point! That was precisely my initial approach (sending an extra Digging up more I realized that by having filenames be sticky to that list of With that in mind, if anyone wants to take a stab at hooking up to the failure paths in both cjs/esm I agree that would be the best approach but otherwise if that turns out to be a dead end then I'd like to consider this as a fix since it unlocks the watcher for a bunch of very useful cases that I'd particularly like to see solved. |
|
@ruyadorno what do you think about watching the entire parent directory of the entry module in case of |
|
@MoLow I get the intent of trying to be as accurate as possible and I can relate to the sentiment that continuously watch previously seen files sounds wrong in theory. With that said, when I think in practical terms I still see it as the best option at hand, or at least the option with least inconveniences. From my understanding the built in I don't really have a strong opinion here 🙃 just trying to explain my current line of thought and I can be easily convinced otherwise. The one thing I have a strong opinion about is that I would prefer a solution that is easier to test, since I'm still very unhappy about the current test included in this PR (which means I'm also very open to different ideas on how to test the current impl). |
|
@ruyadorno I assume we can try and make something more accurate than watching the entire directory, but I agree with you that this should be fixed. |
TLDR; I was trying to use
--watchin a JS project that requires code from abuild/*.jsmodule that is transpiled with tsc but the watcher would not trigger a reloading once the file was regenerated. Having a sticky list of filtered files fixes that.When watching folders that contain files that result from a build step, e.g: TypeScript output, stored template result, etc. The watcher will not reload the application in case one of these resulting files gets regenerated, given that when these files are removed they're no longer tracked by the watcher.
This changeset enables reloading the app when changing files that result from a build step by removing the reset of the tracked filtered files on every reload. Fixing the ability to watch and reload the app when a previously-known output file is changed.
cc @MoLow