-
-
Notifications
You must be signed in to change notification settings - Fork 390
chore: update defaults #552
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.
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #552 +/- ##
==========================================
- Coverage 88.55% 86.92% -1.63%
==========================================
Files 5 6 +1
Lines 428 520 +92
Branches 96 129 +33
==========================================
+ Hits 379 452 +73
- Misses 47 63 +16
- Partials 2 5 +3
Continue to review full report at Codecov.
|
4a6e9fb to
66d43fa
Compare
d7ddfa9 to
d6736cc
Compare
4292ddb to
ffded7a
Compare
src/index.js
Outdated
| chunk.filenameTemplate || chunk.hasRuntime() | ||
| ? this.options.filename | ||
| : this.options.chunkFilename; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| chunk.filenameTemplate || chunk.hasRuntime() | |
| ? this.options.filename | |
| : this.options.chunkFilename; | |
| chunk.filenameTemplate || (chunk.hasRuntime() || chunk.isOnlyInitial() | |
| ? this.options.filename | |
| : this.options.chunkFilename); |
src/index.js
Outdated
| updateHash(hash, chunkGraph) { | ||
| super.updateHash(hash, chunkGraph); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| updateHash(hash, chunkGraph) { | |
| super.updateHash(hash, chunkGraph); | |
| updateHash(hash, context) { | |
| super.updateHash(hash, context); |
src/index.js
Outdated
| ) { | ||
| webpack.javascript.JavascriptModulesPlugin.getCompilationHooks( | ||
| compilation | ||
| ).chunkHash.tap(pluginName, (chunk, hash) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be unneeded once the logic uses a RuntimeModule as the content of runtime modules is hashed and added to the chunk hash automatically
src/index.js
Outdated
|
|
||
| const { mainTemplate } = compilation; | ||
|
|
||
| mainTemplate.hooks.localVars.tap(pluginName, (source, chunk) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this runtime code should use a RuntimeModule in webpack 5.
Something like CssChunkLoadingRuntimeModule.
A runtime module can be added like that:
compilation.hooks.runtimeRequirementInTree
.for(RuntimeGlobals.ensureChunkHandlers)
.tap(pluginName, (chunk, set) => {
compilation.addRuntimeModule(chunk, new CssChunkLoadingRuntimeModule());
});Take a look at ReadFileChunkLoadingRuntimeModule in webpack for an example how to do that.
Technically you can put the HMR logic there too, but we can leave this for another PR.
d20b810 to
c2ad59d
Compare
src/CssChunkLoadingRuntimeModule.js
Outdated
| `${fn}.n = ${runtimeTemplate.basicFunction( | ||
| 'chunkId, promises', | ||
| Template.indent([ | ||
| // source, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sokra Should we add sourse here for webpack5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no
|
@sokra After updating hooks, hmr broke in webpack 5. What could be the reason? 「wdm」: caused by plugins in Compilation.hooks.processAssets
Error: Path variable [contenthash] not implemented in this context: [contenthash].css
at fn (/media/veracrypt1/OS/mini-css-extract-plugin/node_modules/webpack/lib/TemplatedPathPlugin.js:52:11)
at fn (/media/veracrypt1/OS/mini-css-extract-plugin/node_modules/webpack/lib/TemplatedPathPlugin.js:40:16)
at /media/veracrypt1/OS/mini-css-extract-plugin/node_modules/webpack/lib/TemplatedPathPlugin.js:275:12
at String.replace (<anonymous>)
at replacePathVariables (/media/veracrypt1/OS/mini-css-extract-plugin/node_modules/webpack/lib/TemplatedPathPlugin.js:268:14)
at Hook.eval [as call] (eval at create (/media/veracrypt1/OS/mini-css-extract-plugin/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:19:10), <anonymous>:5:16)
at Compilation.getAssetPathWithInfo (/media/veracrypt1/OS/mini-css-extract-plugin/node_modules/webpack/lib/Compilation.js:2860:40)
at Compilation.getPathWithInfo (/media/veracrypt1/OS/mini-css-extract-plugin/node_modules/webpack/lib/Compilation.js:2836:15)
at /media/veracrypt1/OS/mini-css-extract-plugin/node_modules/webpack/lib/HotModuleReplacementPlugin.js:460:27
at Hook.eval [as callAsync] (eval at create (/media/veracrypt1/OS/mini-css-extract-plugin/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:7:1)
ℹ 「wdm」: wait until bundle finished: /dist/a02929665a080f5a7082.hot-update.json |
sokra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for a first step. There are probably a few places where webpack 5 support can be improved, but this seem to good to merge after these nitpicks.
src/CssChunkLoadingRuntimeModule.js
Outdated
| `${fn}.n = ${runtimeTemplate.basicFunction( | ||
| 'chunkId, promises', | ||
| Template.indent([ | ||
| // source, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no
src/loader.js
Outdated
| }); | ||
| }); | ||
| }); | ||
| delete this._compilation.assets[childFilename]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not remove things like source map, .gz version, etc. I like the old way more, which removes all chunk related assets. Was there something broken with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it was not. Just a warning about a deprecated hook.
I redid it as it was with the replacement of the hook.
src/CssChunkLoadingRuntimeModule.js
Outdated
| const fn = RuntimeGlobals.ensureChunkHandlers; | ||
| const { compilation, chunk } = this; | ||
| const { runtimeTemplate } = compilation; | ||
| const withCompat = this.runtimeRequirements.has( |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
src/CssChunkLoadingRuntimeModule.js
Outdated
| ), | ||
| '}', | ||
| '', | ||
| withCompat |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| this.buildMeta = {}; | ||
| callback(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CssModule should implement codeGeneration() and getSourceTypes()
getSourceTypes() should return a special source type for css e. g. MODULE_TYPE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
codeGeneration, now always returns an empty sources. What should codeGeneration generate in our case, when we indicate getSourceTypes()?
Сould you please explain in more detail?
// codeGeneration now returns empty sources
{
sources: Map {},
runtimeRequirements: Set { 'module', '__webpack_exports__', '__webpack_require__' }
}| @@ -1,7 +1,3755 @@ | |||
| /******/ (() => { // webpackBootstrap | |||
| /******/ /************************************************************************/ | |||
| /******/ var __webpack_modules__ = ([ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think snapshotting to complete output file is a good idea. Neither for 4 or 5.
This happens when |
Actually you already do that correctly. Seem like this is not called for HotUpdateChunk, which mean The reason why it's breaking here is:
In general this is how HMR works usually for these kind of plugins for webpack 5: In the RuntimeModule the plugin would add an handler for In this handler they also push an In this applyHandler the runtime code does the necessary updates to the modules to reflect the new state. This plugin can also use this to implement HMR.
|
This might be implemented in another PR? @evilebottnawi |
Co-authored-by: Tobias Koppers <tobias.koppers@googlemail.com>
|
Close in favor #602 |
This PR contains a:
Motivation / Use-Case
hmroption in favor Hot Module ReplacementreloadAlloptionmoduleFilenameoption in favorfilenameoptionBreaking Changes
hmroption in favorHot Module ReplacementreloadAlloptionmoduleFilenameoption in favorfilenameoptionAdditional Info
No