fix: prevent empty JS file generation for CSS-only entry points#20454
fix: prevent empty JS file generation for CSS-only entry points#20454alexander-akait merged 8 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: cd1b51b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This PR is packaged and the instant preview is available (6dec682). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@6dec682
yarn add -D webpack@https://pkg.pr.new/webpack@6dec682
pnpm add -D webpack@https://pkg.pr.new/webpack@6dec682 |
| `${i}/entry2.${ext}`, | ||
| `${i}/runtime~entry1.${ext}`, | ||
| `${i}/runtime~entry2.${ext}` | ||
| ]; |
There was a problem hiding this comment.
now we don't generate extra runtime code for such cases because here we have css entries here, right?
There was a problem hiding this comment.
Yeah — in both of the following entry cases, we no longer generate runtime.js or main.js.
entry: {
app: ["../_images/file.png", "./entry.css"]
}| } | ||
| } | ||
| ], | ||
| sideEffects: true |
There was a problem hiding this comment.
Do we really need sideEffects here? Because no one developer set it manually
| if (innerCache === undefined) { | ||
| innerCache = new WeakMap(); | ||
| chunkHasJsCache.set(chunkGraph, innerCache); | ||
| } |
There was a problem hiding this comment.
Do we really need to use ChunkGraph as a key here (i.e. two level cache)? I think ChunkGraph is not changing due compilations... in case of multi compiler I think we can move chunkHasJsCache inside class, i.e. this._chunkHasJsCache = new WeakMap
Ideally in future we should reduce count of new WeakMap/new WeakSet/new Map/new Set and etc at the top of file, it takes time for initial loading, yeah, it is not critical, but we should refactor it in future
There was a problem hiding this comment.
The two-level WeakMap cache is necessary since each compilation (including HMR) creates a new ChunkGraph instance, so the ChunkGraph key correctly isolates results per compilation.
We cannot move the cache into the class because chunkHasJs is a static method.
There was a problem hiding this comment.
Got it, we will think how to refactor this to avoid extra initial maps/sets in future
alexander-akait
left a comment
There was a problem hiding this comment.
Looks very good, let's resolve couple things and we can merge
|
@xiaoxiaojx Looks like some tests are freezes on old node versions, maybe we need to exclude old versions for these tests |
| // which occurs because the default `output.library` setting is `commonjs2`, | ||
| // resulting in adding `module.exports = __webpack_exports__;`. | ||
| set.add(webpack.RuntimeGlobals.startup); | ||
| set.add(webpack.RuntimeGlobals.exports); |
There was a problem hiding this comment.
Yes, it's code copied from another test case. Honestly, I didn't really notice what it does. 😂
https://github.com/webpack/webpack/blob/main/test/hotCases/css/single-css-entry/webpack.config.js#L23
There was a problem hiding this comment.
Yeah, will be great to understand this and maybe remove in future, but let's do it separately
Summary
Fixes #11671
Fixes #20427
What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Yes
Does this PR introduce a breaking change?
No
If relevant, what needs to be documented once your changes are merged or what have you already documented?
Nothing