feat: watch mode config change #2797
Conversation
| @@ -1581,7 +1580,7 @@ class WebpackCLI { | |||
| return loadedConfig; | |||
| }; | |||
|
|
|||
| const config = { options: {}, path: new WeakMap() }; | |||
| const config = { options: {}, path: new Map() }; | |||
There was a problem hiding this comment.
We're not using config.path anywhere, what's to leak?
webpack-bot
added
PR: review-outdated
and removed
PR: reviewed-changes-requested
labels
|
@evenstensberg Thanks for your update. I labeled the Pull Request so reviewers will review it again. @alexander-akait Please review the new changes. |
|
PTAL |
| async tryRequireThenImport(module, handleError = true) { | ||
| let result; | ||
|
|
||
| try { | ||
| delete require.cache[module]; |
There was a problem hiding this comment.
Unsafe and very bad for perf
There was a problem hiding this comment.
its cache for one file, perf wont change that much because of this
| ); | ||
| if (alreadyHasPLugin) { | ||
| return configOptions; | ||
| } |
There was a problem hiding this comment.
Rerun applyOptions is very bad idea, it has very bad performance and it is not safe too
|
|
||
| if (options.watch) { | ||
| // eslint-disable-next-line | ||
| for (const [_, filePath] of compiler.config.path.entries()) { |
There was a problem hiding this comment.
You can do the same using WeakMap
There was a problem hiding this comment.
cant iterate over a weakmap
There was a problem hiding this comment.
You have configuration keys
| for (const [_, filePath] of compiler.config.path.entries()) { | ||
| // eslint-disable-next-line | ||
| fs.watchFile(filePath, async () => { | ||
| compiler = await this.createCompiler(options, callback); |
There was a problem hiding this comment.
It should rerun commander, not create compiler
There was a problem hiding this comment.
Ideally we should not rerun command whole, webpack can revalidate own options and run new compilation(s), everything webpack needs to know is path to configuration(s) for each compiler, so this task should be done on webpack side, not here, to be honestly it is not hard to implement, medium difficult
|
@alexander-akait you have the source, maybe you can have a look at it? |
|
Some points:
Due this, I think this is the right level of abstraction. Since the CLI is the one that reads a config, I think this is where we naturally add this feature. I don't understand why webpack itself should re-validate a config, core should only be worried about doing a compilation. WDYT? |
|
@evenstensberg Because only webpack knows about dependencies |
|
We just want config changes, not dependency changes. Watchmode + watch a config file |
|
In this case we don't look at |
|
The watch API will sniff those changes if the text inside the config changes. this is what we want |
|
We should watch dependencies too like |
|
No need, if we refer to the original issue |
|
It is require to right implementation |
|
I wanted to close it, because it is invalid, we need to improve it on webpack side, we need:
|
What kind of change does this PR introduce?
Fixes #15
Did you add tests for your changes?
No
If relevant, did you update the documentation?
N/A
Summary
Need to check if CLIplugin is added to the config, otherwise there will be a memory leak
Does this PR introduce a breaking change?
No
Other information
N/A