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.

Sign up for GitHub

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

Jump to bottom

fix suspend and resuming watching correctly #13399

Merged
merged 4 commits into from May 19, 2021
Merged

fix suspend and resuming watching correctly #13399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
749cfa6
add test cases from #12897 and #12881
sokra May 17, 2021
23728e1
improve test case
sokra May 18, 2021
0d4dbd0
fix collecting of changed files and aggregating when suspending
sokra May 18, 2021
23f492f
update typings
sokra May 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
103 changes: 71 additions & 32 deletions lib/Watching.js
View file
Open in desktop
Expand Up @@ -57,16 +57,53 @@ class Watching {
this._needRecords = true;
this.watcher = undefined;
this.pausedWatcher = undefined;
/** @type {Set<string>} */
this._collectedChangedFiles = undefined;
/** @type {Set<string>} */
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 => {
Expand All @@ -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 => {
Expand Down Expand Up @@ -250,25 +300,19 @@ class Watching {
changedFiles,
removedFiles
) => {
this.pausedWatcher = this.watcher;
this.watcher = null;
if (err) {
this.compiler.modifiedFiles = undefined;
this.compiler.removedFiles = undefined;
this.compiler.fileTimestamps = undefined;
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) => {
Expand Down Expand Up @@ -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
);
}
}

Expand Down
23 changes: 19 additions & 4 deletions lib/node/NodeWatchFileSystem.js
View file
Open in desktop
Expand Up @@ -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();
Expand All @@ -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) {
Expand Down
81 changes: 81 additions & 0 deletions test/WatchSuspend.test.js
View file
Open in desktop
Expand Up @@ -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;
Expand All @@ -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
}
Expand All @@ -45,6 +49,7 @@ describe("WatchSuspend", () => {
}
});
watching = compiler.watch({ aggregateTimeout: 50 }, () => {});

compiler.hooks.done.tap("WatchSuspendTest", () => {
if (onChange) onChange();
});
Expand Down Expand Up @@ -92,5 +97,81 @@ describe("WatchSuspend", () => {
};
watching.resume();
});

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;
// 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);
};
});
});
});
4 changes: 2 additions & 2 deletions types.d.ts
View file
Open in desktop
Expand Up @@ -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<string>,
dirs: Iterable<string>,
Expand Down