Update webpack to version 5 #195
Conversation
Codecov Report
@@ Coverage Diff @@
## master #195 +/- ##
===========================================
+ Coverage 98.86% 100.00% +1.13%
===========================================
Files 1 1
Lines 88 76 -12
Branches 30 27 -3
===========================================
- Hits 87 76 -11
+ Misses 1 0 -1
Continue to review full report at Codecov.
|
|
wow! great work here and thank you for the contribution 👏 I'm not currently using webpack to validate we're all good here, but the code changes look good. Would be great to get some other eyes on this before bringing it in. cc @chrisblossom |
|
I'm not really sure how to fix the coverage report. I've added some code to make typescript happy (for selecting the related assets) but I'm not really sure how to trigger that which webpack. Tips are appreciated |
package.json
Outdated
| }, | ||
| "dependencies": { | ||
| "@types/webpack": "^4.4.31", | ||
| "@types/webpack": "^4.41.25", |
There was a problem hiding this comment.
You may want to remove this dependency since webpack 5 has it's own typescript types which conflict with these types. This is also the reason why a lot of people have troubles using webpack 5 with typescript because plugins stil have this package as a dependency.
There was a problem hiding this comment.
How will this affect people not using webpack 5? Does that even matter?
There was a problem hiding this comment.
Think you need to make a decision if you want to typecheck the webpack 5 code or the webpack 4 code. if you want to keep type checking webpack 4 code you should move this to dev dependencies so webpack 5 users won't have conflicts.
But i'm not totally sure about this. In my recent projects I only supported webpack 5 from a certain version and not both at the same time.
should also be changed to for compatibility with type declarations provided by webpack 5, see #193. |
|
@djcsdy I think I did that already. Could you specify in which file I've missed it? |
|
@strootje My mistake, you're right, it's already done. I must have been looking at the wrong commit. |
|
The README.md still mentions
Just to mention it, the zero configuration doesn't work anymore with this plugin and v5 (probably the reason why you had to limit that test case to webpack v4). Mentioned the reason for this here: #181 (comment). |
src/clean-webpack-plugin.ts
Outdated
| const assets = | ||
| stats.toJson( | ||
| { | ||
| assets: true, | ||
| }, | ||
| true, | ||
| ).assets || []; | ||
| const assetList = assets.map((asset: { name: string }) => { | ||
| stats.toJson({ | ||
| assets: true, | ||
| }).assets || []; | ||
|
|
||
| const relatedAssets = assets | ||
| .map((asset) => { | ||
| return ( | ||
| (asset.related && | ||
| Array.isArray(asset.related) && | ||
| asset.related) || | ||
| [] | ||
| ); | ||
| }) | ||
| .reduce((previousAssets, currentAssets) => { | ||
| return [...previousAssets, ...currentAssets]; | ||
| }, []); | ||
|
|
||
| const allAssets = [...assets, ...relatedAssets]; | ||
| const assetList = allAssets.map((asset) => { | ||
| return asset.name; | ||
| }); |
There was a problem hiding this comment.
I think you can just use
const assetList = Object.keys(stats.compilation.assets);There was a problem hiding this comment.
Thanks for the suggestion. This seems to be working and let's me throw almost all my code away! 👍
I think the reason for this is that you removed some code which changed the ratio between tested and untested code. Looks like there is only one line/case that isn't covered by tests see https://codecov.io/gh/johnagan/clean-webpack-plugin/src/b2dccbbcd626a151772332ec1ba567473458a8c2/src/clean-webpack-plugin.ts#L340 You can try changing it to /* istanbul ignore next */
throw error;I checked del/rimraf and looks like they handle errors quite well so I'd say it's not that easy to build a test for this. |
|
@chrisblossom and @johnagan any chance for a rereview? |
|
@strootje nice work! I'll merge it in 🎉 |
Updated to webpack version 5.
I had to drop support for webpack version 3 and node < 10.