Skip to content

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

Closed
wants to merge 9 commits into from
Closed

Conversation

cap-Bernardito
Copy link
Member

@cap-Bernardito cap-Bernardito commented Jul 20, 2020

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

  • update defaults
  • update dependencies
  • remove hmr option in favor Hot Module Replacement
  • remove reloadAll option
  • remove moduleFilename option in favor filename option
  • update deprecated hooks for webpack 5

Breaking Changes

  • remove hmr option in favor Hot Module Replacement
  • remove reloadAll option
  • remove moduleFilename option in favor filename option

Additional Info

No

@codecov
Copy link

codecov bot commented Jul 20, 2020

Codecov Report

Merging #552 into master will decrease coverage by 1.62%.
The diff coverage is 82.06%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
src/CssChunkLoadingRuntimeModule.js 55.88% <55.88%> (ø)
src/index.js 89.10% <84.82%> (+1.25%) ⬆️
src/loader.js 90.90% <97.05%> (+1.43%) ⬆️
src/hmr/hotModuleReplacement.js 86.11% <100.00%> (-1.86%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b146549...6e56877. Read the comment docs.

@cap-Bernardito cap-Bernardito force-pushed the chore-update-defaults branch from 4a6e9fb to 66d43fa Compare July 20, 2020 13:28
@cap-Bernardito cap-Bernardito force-pushed the chore-update-defaults branch from d7ddfa9 to d6736cc Compare July 21, 2020 16:53
@cap-Bernardito cap-Bernardito force-pushed the chore-update-defaults branch from 4292ddb to ffded7a Compare July 23, 2020 17:21
@cap-Bernardito cap-Bernardito marked this pull request as ready for review July 23, 2020 17:24
src/index.js Outdated
Comment on lines 243 to 245
chunk.filenameTemplate || chunk.hasRuntime()
? this.options.filename
: this.options.chunkFilename;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Comment on lines 98 to 99
updateHash(hash, chunkGraph) {
super.updateHash(hash, chunkGraph);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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) => {
Copy link
Member

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
@@ -240,7 +337,7 @@ class MiniCssExtractPlugin {
const { mainTemplate } = compilation;

mainTemplate.hooks.localVars.tap(pluginName, (source, chunk) => {
Copy link
Member

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.

@cap-Bernardito cap-Bernardito force-pushed the chore-update-defaults branch from d20b810 to c2ad59d Compare July 24, 2020 17:30
`${fn}.n = ${runtimeTemplate.basicFunction(
'chunkId, promises',
Template.indent([
// source,
Copy link
Member Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no

@cap-Bernardito
Copy link
Member Author

@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

Copy link
Member

@sokra sokra left a 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.

`${fn}.n = ${runtimeTemplate.basicFunction(
'chunkId, promises',
Template.indent([
// source,
Copy link
Member

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];
Copy link
Member

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?

Copy link
Member Author

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.

const fn = RuntimeGlobals.ensureChunkHandlers;
const { compilation, chunk } = this;
const { runtimeTemplate } = compilation;
const withCompat = this.runtimeRequirements.has(

This comment was marked as resolved.

),
'}',
'',
withCompat

This comment was marked as resolved.

@@ -83,8 +95,8 @@ class CssModule extends webpack.Module {
callback();
}

Copy link
Member

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.

Copy link
Member Author

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__ = ([
Copy link
Member

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.

@sokra
Copy link
Member

sokra commented Jul 31, 2020

@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

This happens when contentHash is undefined for a chunk. Here a HotUpdateChunk. Usually plugins set the contentHash in Compilation.hooks.contenthash. You should do the same for all CssModules in a chunk.

@sokra
Copy link
Member

sokra commented Jul 31, 2020

This happens when contentHash is undefined for a chunk. Here a HotUpdateChunk. Usually plugins set the contentHash in Compilation.hooks.contenthash. You should do the same for all CssModules in a chunk.

Actually you already do that correctly. Seem like this is not called for HotUpdateChunk, which mean [contenthash] can't be used for HotUpdateChunks. This probably also doesn't work for normal javascript when hotUpdateChunkFilename contains [contenthash].

The reason why it's breaking here is:

  • renderManifest is also called for HotUpdateChunk
  • Plugins are expected to generate an hot update instead of a normal chunk.
  • These chunks use a different filenameTemplate -> output.hotUpdateChunkFilename
  • As we can't generate a css patch anyway, the plugin should do nothing for HotUpdateChunks in renderManifest
  • It won't download these patches anyway as the HMR code is missing in the RuntimeModule
  • That should resolve the problem.

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 RuntimeGlobals.hmrDownloadUpdateHandlers to download the hot update chunk when requested. This is very similar to RuntimeGlobals.ensureChunkHandlers gets trigger by import().

In this handler they also push an applyHandler to a list, which gets called once the HMR update should be applied.

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.

  • Register a hmrDownloadUpdateHandler which loads the updated stylesheet in a disabled link tag (media="never").
  • In the apply handler enable the link tag and remove the old one.

@sokra
Copy link
Member

sokra commented Jul 31, 2020

This plugin can also use this to implement HMR.

  • Register a hmrDownloadUpdateHandler which loads the updated stylesheet in a disabled link tag (media="never").
  • In the apply handler enable the link tag and remove the old one.

This might be implemented in another PR? @evilebottnawi

cap-Bernardito and others added 2 commits August 3, 2020 12:24
@alexander-akait
Copy link
Member

Close in favor #602

@alexander-akait alexander-akait deleted the chore-update-defaults branch November 4, 2020 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants