Fix multiconfig watch #3737
Fix multiconfig watch #3737
Conversation
Previously, stats would contain stale configuration child stats. This is very confusing, since it looks as if some things are emitted when they shouldn't be. Add support for making assertions to stats in watch tests.
yuriyostapenko
force-pushed
the
fix-multiconfig-watch
branch
from
120bdfb
to
93d85bf
Compare
|
@TheLarkInn, are you sure the label is correct or did it just get mass labeled by mistake? |
|
Ah yes I did sorry I'll remove that. Was doing batch updates and forgot to skip one! |
| if(compilerStatus.every(Boolean)) { | ||
| var multiStats = new MultiStats(allStats); | ||
| var freshStats = allStats.filter(function(s) { |
There was a problem hiding this comment.
allStats.filter(function(s, idx) {
return compilerStatus[idx] === "new";
}There was a problem hiding this comment.
the hash is not really needed. Could you simplify it? See my code examples.
| @@ -143,10 +143,14 @@ MultiCompiler.prototype.watch = function(watchOptions, handler) { | |||
| handler(err); | |||
| if(stats) { | |||
| allStats[compilerIdx] = stats; | |||
| compilerStatus[compilerIdx] = true; | |||
| compilerStatus[compilerIdx] = stats.hash; | |||
There was a problem hiding this comment.
compilerStatus[compilerIdx] = "new"
| handler(null, multiStats) | ||
| compilerStatus.fill(true); |
There was a problem hiding this comment.
Could you move this line two line up? If someone doing weird stuff in the handler.
| output: { | ||
| filename: "./static.js" | ||
| } | ||
| }]; |
There was a problem hiding this comment.
Could you add another config. And a watch step in which two configuration change at once.
There was a problem hiding this comment.
That wouldn't work.
I mean that currently (including before my changes), only the first MultiStats is really batched, after that, if(compilerStatus.every(Boolean)) is always satisfied and each of the stats is forwarded to handler independently.
Here yuriyostapenko@d4a7aac is what I tried and it doesn't work.
Compilations do not share watchers, so I guess you'd need to aggregate them somehow.
But I think it's fine if these stats are sent to handler one by one in watch mode, don't you?
Parallel watchpack's (happens when MultiCompiler is in use) do not emit events in watch tests, unless some timeout is added. This does not seem to be an issue on macOS.
This fixes macOS build, where changes keep being triggered with parallel watchers
yuriyostapenko
force-pushed
the
fix-multiconfig-watch
branch
from
93d85bf
to
9a688a4
Compare
TheLarkInn
dismissed
sokra’s stale review
Linting now passes and I will merge.
|
Nice work. Thank you so much!! 👏 |
What kind of change does this PR introduce?
bugfix
Did you add tests for your changes?
yes
If relevant, link to documentation update:
Summary
Multi configuration watch is reporting stale stats for configurations that were not changed
Does this PR introduce a breaking change?
no
Other information
There is some quirk about running watch tests on Windows/Linux. Change event doesn't fire. It is only observable when MultiCompilation is in use (probably because it uses parallel watchpacks). It is mitigated by adding small delay before changing the files, which is also part of this PR.