From 749cfa6924973b3c3bc08621c94469207ab740ff Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Mon, 17 May 2021 16:36:11 +0200 Subject: [PATCH 1/4] add test cases from #12897 and #12881 Co-authored-by: Vladimir Razuvaev --- test/WatchSuspend.test.js | 62 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/test/WatchSuspend.test.js b/test/WatchSuspend.test.js index a4cdc67a811..3dbcd34e4e5 100644 --- a/test/WatchSuspend.test.js +++ b/test/WatchSuspend.test.js @@ -20,6 +20,8 @@ describe("WatchSuspend", () => { "temp-watch-" + Date.now() ); const filePath = path.join(fixturePath, "file.js"); + const file2Path = path.join(fixturePath, "file2.js"); + const file3Path = path.join(fixturePath, "file3.js"); const outputPath = path.join(fixturePath, "bundle.js"); let compiler = null; let watching = null; @@ -33,6 +35,8 @@ describe("WatchSuspend", () => { } try { fs.writeFileSync(filePath, "'foo'", "utf-8"); + fs.writeFileSync(file2Path, "'file2'", "utf-8"); + fs.writeFileSync(file3Path, "'file3'", "utf-8"); } catch (e) { // skip } @@ -45,6 +49,7 @@ describe("WatchSuspend", () => { } }); watching = compiler.watch({ aggregateTimeout: 50 }, () => {}); + compiler.hooks.done.tap("WatchSuspendTest", () => { if (onChange) onChange(); }); @@ -92,5 +97,62 @@ describe("WatchSuspend", () => { }; watching.resume(); }); + + it("should not ignore changes during resumed compilation", async () => { + // aggregateTimeout must be long enough for this test + // So set-up new watcher and wait when initial compilation is done + await new Promise(resolve => { + watching.close(); + watching = compiler.watch({ aggregateTimeout: 1000 }, resolve); + }); + return new Promise(resolve => { + watching.suspend(); + fs.writeFileSync(filePath, "'baz'", "utf-8"); + + // Run resume between "changed" and "aggregated" events + setTimeout(() => { + watching.resume(); + + setTimeout(() => { + expect(fs.readFileSync(outputPath, "utf-8")).toContain("'baz'"); + resolve(); + }, 2000); + }, 200); + }); + }); + + it("should not drop changes when suspended", done => { + const aggregateTimeout = 50; + // Trigger initial compilation with file2.js (assuming correct) + fs.writeFileSync( + filePath, + 'require("./file2.js"); require("./file3.js")', + "utf-8" + ); + + onChange = () => { + // Initial compilation is done, start the test + watching.suspend(); + + // Trigger the first change (works as expected): + fs.writeFileSync(file2Path, "'foo'", "utf-8"); + + // Trigger the second change _after_ aggregation timeout of the first + setTimeout(() => { + fs.writeFileSync(file3Path, "'bar'", "utf-8"); + + // Wait when the file3 edit is settled and re-compile + setTimeout(() => { + watching.resume(); + + onChange = () => { + onChange = null; + expect(fs.readFileSync(outputPath, "utf-8")).toContain("'bar'"); + done(); + }; + }, 200); + }, aggregateTimeout + 50); + }; + }); }); }); From 23728e10bd23fed8bcecaf56ff5a5c030d86dfc0 Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Tue, 18 May 2021 13:25:27 +0200 Subject: [PATCH 2/4] improve test case --- test/WatchSuspend.test.js | 63 +++++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 22 deletions(-) diff --git a/test/WatchSuspend.test.js b/test/WatchSuspend.test.js index 3dbcd34e4e5..df2fa018a6f 100644 --- a/test/WatchSuspend.test.js +++ b/test/WatchSuspend.test.js @@ -98,28 +98,47 @@ describe("WatchSuspend", () => { watching.resume(); }); - it("should not ignore changes during resumed compilation", async () => { - // aggregateTimeout must be long enough for this test - // So set-up new watcher and wait when initial compilation is done - await new Promise(resolve => { - watching.close(); - watching = compiler.watch({ aggregateTimeout: 1000 }, resolve); - }); - return new Promise(resolve => { - watching.suspend(); - fs.writeFileSync(filePath, "'baz'", "utf-8"); - - // Run resume between "changed" and "aggregated" events - setTimeout(() => { - watching.resume(); - - setTimeout(() => { - expect(fs.readFileSync(outputPath, "utf-8")).toContain("'baz'"); - resolve(); - }, 2000); - }, 200); - }); - }); + for (const changeBefore of [false, true]) + for (const delay of [200, 1500]) { + // eslint-disable-next-line no-loop-func + it(`should not ignore changes during resumed compilation (changeBefore: ${changeBefore}, delay: ${delay}ms)`, async () => { + // aggregateTimeout must be long enough for this test + // So set-up new watcher and wait when initial compilation is done + await new Promise(resolve => { + watching.close(() => { + watching = compiler.watch({ aggregateTimeout: 1000 }, () => { + resolve(); + }); + }); + }); + return new Promise(resolve => { + if (changeBefore) fs.writeFileSync(filePath, "'bar'", "utf-8"); + setTimeout(() => { + watching.suspend(); + fs.writeFileSync(filePath, "'baz'", "utf-8"); + + onChange = "throw"; + setTimeout(() => { + onChange = () => { + expect(fs.readFileSync(outputPath, "utf-8")).toContain( + "'baz'" + ); + expect( + compiler.modifiedFiles && + Array.from(compiler.modifiedFiles).sort() + ).toEqual([filePath]); + expect( + compiler.removedFiles && Array.from(compiler.removedFiles) + ).toEqual([]); + onChange = null; + resolve(); + }; + watching.resume(); + }, delay); + }, 200); + }); + }); + } it("should not drop changes when suspended", done => { const aggregateTimeout = 50; From 0d4dbd019a98ac12545b02831795a00bfa548c70 Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Tue, 18 May 2021 13:31:02 +0200 Subject: [PATCH 3/4] fix collecting of changed files and aggregating when suspending --- lib/Watching.js | 103 ++++++++++++++++++++++---------- lib/node/NodeWatchFileSystem.js | 23 +++++-- 2 files changed, 90 insertions(+), 36 deletions(-) diff --git a/lib/Watching.js b/lib/Watching.js index 61a747d73fa..046382c09a4 100644 --- a/lib/Watching.js +++ b/lib/Watching.js @@ -57,16 +57,53 @@ class Watching { this._needRecords = true; this.watcher = undefined; this.pausedWatcher = undefined; + /** @type {Set} */ + this._collectedChangedFiles = undefined; + /** @type {Set} */ + this._collectedRemovedFiles = undefined; this._done = this._done.bind(this); process.nextTick(() => { if (this._initial) this._invalidate(); }); } - _go() { + _mergeWithCollected(changedFiles, removedFiles) { + if (!changedFiles) return; + if (!this._collectedChangedFiles) { + this._collectedChangedFiles = new Set(changedFiles); + this._collectedRemovedFiles = new Set(removedFiles); + } else { + for (const file of changedFiles) { + this._collectedChangedFiles.add(file); + this._collectedRemovedFiles.delete(file); + } + for (const file of removedFiles) { + this._collectedChangedFiles.delete(file); + this._collectedRemovedFiles.add(file); + } + } + } + + _go(fileTimeInfoEntries, contextTimeInfoEntries, changedFiles, removedFiles) { this._initial = false; this.startTime = Date.now(); this.running = true; + if (this.watcher) { + this.pausedWatcher = this.watcher; + this.watcher.pause(); + this.watcher = null; + } + this._mergeWithCollected( + changedFiles || + (this.pausedWatcher && + this.pausedWatcher.getAggregatedChanges && + this.pausedWatcher.getAggregatedChanges()), + (this.compiler.removedFiles = + removedFiles || + (this.pausedWatcher && + this.pausedWatcher.getAggregatedRemovals && + this.pausedWatcher.getAggregatedRemovals())) + ); const run = () => { if (this.compiler.idle) { return this.compiler.cache.endIdle(err => { @@ -83,6 +120,19 @@ class Watching { run(); }); } + + this.compiler.modifiedFiles = this._collectedChangedFiles; + this._collectedChangedFiles = undefined; + this.compiler.removedFiles = this._collectedRemovedFiles; + this._collectedRemovedFiles = undefined; + + this.compiler.fileTimestamps = + fileTimeInfoEntries || + (this.pausedWatcher && this.pausedWatcher.getFileTimeInfoEntries()); + this.compiler.contextTimestamps = + contextTimeInfoEntries || + (this.pausedWatcher && this.pausedWatcher.getContextTimeInfoEntries()); + this.invalid = false; this._invalidReported = false; this.compiler.hooks.watchRun.callAsync(this.compiler, err => { @@ -250,8 +300,6 @@ class Watching { changedFiles, removedFiles ) => { - this.pausedWatcher = this.watcher; - this.watcher = null; if (err) { this.compiler.modifiedFiles = undefined; this.compiler.removedFiles = undefined; @@ -259,16 +307,12 @@ class Watching { this.compiler.contextTimestamps = undefined; return this.handler(err); } - this.compiler.fileTimestamps = fileTimeInfoEntries; - this.compiler.contextTimestamps = contextTimeInfoEntries; - this.compiler.removedFiles = removedFiles; - this.compiler.modifiedFiles = changedFiles; - if (this.watcher) { - this.pausedWatcher = this.watcher; - this.watcher.pause(); - this.watcher = null; - } - this._invalidate(); + this._invalidate( + fileTimeInfoEntries, + contextTimeInfoEntries, + changedFiles, + removedFiles + ); this._onChange(); }, (fileName, changeTime) => { @@ -296,31 +340,26 @@ class Watching { this._invalidate(); } - _invalidate() { - if (this.suspended) return; - if (this._isBlocked()) { - this.blocked = true; + _invalidate( + fileTimeInfoEntries, + contextTimeInfoEntries, + changedFiles, + removedFiles + ) { + if (this.suspended || (this._isBlocked() && (this.blocked = true))) { + this._mergeWithCollected(changedFiles, removedFiles); return; } - if (this.watcher) { - this.compiler.modifiedFiles = - this.watcher.getAggregatedChanges && - this.watcher.getAggregatedChanges(); - this.compiler.removedFiles = - this.watcher.getAggregatedRemovals && - this.watcher.getAggregatedRemovals(); - this.compiler.fileTimestamps = this.watcher.getFileTimeInfoEntries(); - this.compiler.contextTimestamps = - this.watcher.getContextTimeInfoEntries(); - this.pausedWatcher = this.watcher; - this.watcher.pause(); - this.watcher = null; - } if (this.running) { this.invalid = true; } else { - this._go(); + this._go( + fileTimeInfoEntries, + contextTimeInfoEntries, + changedFiles, + removedFiles + ); } } diff --git a/lib/node/NodeWatchFileSystem.js b/lib/node/NodeWatchFileSystem.js index 5e2421bd964..67ef918b8a3 100644 --- a/lib/node/NodeWatchFileSystem.js +++ b/lib/node/NodeWatchFileSystem.js @@ -70,11 +70,12 @@ class NodeWatchFileSystem { } this.watcher.once("aggregated", (changes, removals) => { if (this.inputFileSystem && this.inputFileSystem.purge) { + const fs = this.inputFileSystem; for (const item of changes) { - this.inputFileSystem.purge(item); + fs.purge(item); } for (const item of removals) { - this.inputFileSystem.purge(item); + fs.purge(item); } } const times = this.watcher.getTimeInfoEntries(); @@ -99,10 +100,24 @@ class NodeWatchFileSystem { } }, getAggregatedRemovals: () => { - return this.watcher && this.watcher.aggregatedRemovals; + const items = this.watcher && this.watcher.aggregatedRemovals; + if (items && this.inputFileSystem && this.inputFileSystem.purge) { + const fs = this.inputFileSystem; + for (const item of items) { + fs.purge(item); + } + } + return items; }, getAggregatedChanges: () => { - return this.watcher && this.watcher.aggregatedChanges; + const items = this.watcher && this.watcher.aggregatedChanges; + if (items && this.inputFileSystem && this.inputFileSystem.purge) { + const fs = this.inputFileSystem; + for (const item of items) { + fs.purge(item); + } + } + return items; }, getFileTimeInfoEntries: () => { if (this.watcher) { From 23f492f8efc9fd60466e087eadeaa2bb798218fe Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Tue, 18 May 2021 16:00:45 +0200 Subject: [PATCH 4/4] update typings --- types.d.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/types.d.ts b/types.d.ts index 98f700cdb7a..afa09280a7f 100644 --- a/types.d.ts +++ b/types.d.ts @@ -11596,8 +11596,8 @@ declare abstract class Watching { }; compiler: Compiler; running: boolean; - watcher: any; - pausedWatcher: any; + watcher?: null | Watcher; + pausedWatcher?: null | Watcher; watch( files: Iterable, dirs: Iterable,