fix: inherit sideEffects past type-only nested package.json - #21686
fix: inherit sideEffects past type-only nested package.json#21686xiaoxiaojx wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 4ba77ec 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 |
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Package metadata resolution lib/optimize/SideEffectsFlagPlugin.js, lib/util/identifier.js, lib/dependencies/ImportMetaGlobHelpers.js |
The plugin resolves inherited sideEffects values while traversing ancestor package metadata. Shared request normalization is exported and reused. |
Module creation integration lib/optimize/SideEffectsFlagPlugin.js |
afterResolve caches resolved metadata, and module creation applies the cached value. |
Nested package regression coverage test/configCases/side-effects/nested-package-json-sideEffects/*, test/configCases/side-effects/nested-package-json-sideEffects-outside-context/*, .changeset/047-inherit-nested-sideEffects.md |
Configuration cases verify nested declarations, glob rebasing, local package metadata, node_modules boundaries, and paths outside the compiler context. The changeset records the patch release. |
🚥 Pre-merge checks | ✅ 3 | ❌ 1
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | ❓ Inconclusive | The title uses valid Conventional Commit syntax and describes the sideEffects inheritance fix, but the branch prefix is not provided for verification. | Provide the branch name or branch prefix to verify that fix matches it. |
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
Comment @coderabbitai help to get the list of available commands.
|
This PR is packaged and the instant preview is available (4ba77ec). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@4ba77ec
yarn add -D webpack@https://pkg.pr.new/webpack@4ba77ec
pnpm add -D webpack@https://pkg.pr.new/webpack@4ba77ec |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21686 +/- ##
==========================================
- Coverage 94.53% 94.53% -0.01%
==========================================
Files 631 631
Lines 82362 82420 +58
Branches 24405 24424 +19
==========================================
+ Hits 77861 77912 +51
- Misses 4501 4508 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Generated code sizeComparing
4 asset(s) changed size
2 runtime(s) changed which runtime modules they carry
Built |
475a3e0 to
4ba77ec
Compare
Merging this PR will degrade performance by 36.21%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | benchmark "many-modules-esm", scenario '{"name":"mode-development","mode":"development"}' |
882.6 KB | 2,299.2 KB | -61.61% |
| ❌ | Memory | benchmark "devtool-eval", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' |
479.2 KB | 942.9 KB | -49.18% |
| ⚡ | Memory | benchmark "css-modules", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' |
1,233.4 KB | 927.1 KB | +33.05% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/nested-package-json-sideEffects (4ba77ec) with main (638ce71)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/optimize/SideEffectsFlagPlugin.js (2)
278-286: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRead
compiler.inputFileSysteminside the hook, not atapplytime.
compiler.inputFileSystemis reassigned during the compiler lifecycle, for example byNodeEnvironmentPluginon watch invalidation and on close. The captured reference can become stale, and the!inputFileSystemguard then silently disables the whole resolution path for the rest of the run.♻️ Proposed change
- const inputFileSystem = compiler.inputFileSystem; compiler.hooks.compilation.tap( PLUGIN_NAME, (compilation, { normalModuleFactory }) => { const moduleGraph = compilation.moduleGraph; normalModuleFactory.hooks.afterResolve.tapAsync( PLUGIN_NAME, (resolveData, callback) => { + const inputFileSystem = compiler.inputFileSystem; if (!inputFileSystem) return callback();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/optimize/SideEffectsFlagPlugin.js` around lines 278 - 286, Update the compilation hook around `normalModuleFactory.hooks.afterResolve` to read `compiler.inputFileSystem` when the `afterResolve` callback executes, rather than capturing it before `compiler.hooks.compilation.tap`. Apply the existing missing-filesystem guard to this freshly read value so resolution continues using the current compiler filesystem after lifecycle reassignment.
170-226: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize the ancestor walk per directory.
afterResolveruns for every resolved module. Every module inside anode_modulespackage that declares nosideEffectsrepeats the full ancestor walk, including onereadJsonper ancestor directory. The result depends only ondescriptionFileRoot, not onrelativePath, so the walk can be cached per directory and reused.Store the resolved owning package root plus its
sideEffectsvalue in aMapkeyed bydescriptionFileRoot(scoped tocompiler.root, likeglobToRegexpCache), then computerelativePathper module from that cached root. This also removes repeated work when the input filesystem is not a caching one.As per coding guidelines: "Evaluate time and memory costs of changes, especially on hot paths."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/optimize/SideEffectsFlagPlugin.js` around lines 170 - 226, Memoize the ancestor walk in the afterResolve path using a Map scoped to compiler.root, keyed by descriptionFileRoot, alongside globToRegexpCache. Cache both the owning package root and its sideEffects value, including no-result cases, then compute each module’s relativePath from the cached root instead of repeating readJson traversal. Preserve existing boundary and error behavior while avoiding unbounded per-module cache growth.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/optimize/SideEffectsFlagPlugin.js`:
- Around line 197-208: Update the ancestor package.json error handling in the
readJson callback to continue walking via next() for missing or unreadable paths
(ENOENT, ENOTDIR, EISDIR, ENAMETOOLONG) and JSON parse SyntaxError, while
preserving callback(err) for other I/O failures. Keep the existing
parent-directory traversal behavior unchanged.
---
Nitpick comments:
In `@lib/optimize/SideEffectsFlagPlugin.js`:
- Around line 278-286: Update the compilation hook around
`normalModuleFactory.hooks.afterResolve` to read `compiler.inputFileSystem` when
the `afterResolve` callback executes, rather than capturing it before
`compiler.hooks.compilation.tap`. Apply the existing missing-filesystem guard to
this freshly read value so resolution continues using the current compiler
filesystem after lifecycle reassignment.
- Around line 170-226: Memoize the ancestor walk in the afterResolve path using
a Map scoped to compiler.root, keyed by descriptionFileRoot, alongside
globToRegexpCache. Cache both the owning package root and its sideEffects value,
including no-result cases, then compute each module’s relativePath from the
cached root instead of repeating readJson traversal. Preserve existing boundary
and error behavior while avoiding unbounded per-module cache growth.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f1d2eb66-2173-428c-83f0-e3ca40f61717
📒 Files selected for processing (3)
.changeset/047-inherit-nested-sideEffects.mdlib/optimize/SideEffectsFlagPlugin.jstest/configCases/side-effects/nested-package-json-sideEffects/index.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
webpack/schema-utils(auto-detected)webpack/tapable(auto-detected)
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/047-inherit-nested-sideEffects.md
- test/configCases/side-effects/nested-package-json-sideEffects/index.js
| readJson(fs, join(fs, dir, "package.json"), (err, data) => { | ||
| if (err) { | ||
| if ("code" in err && err.code === "ENOENT") { | ||
| dir = parent; | ||
| return next(); | ||
| } | ||
| return callback(err); | ||
| } | ||
| if (!data || typeof data !== "object" || Array.isArray(data)) { | ||
| dir = parent; | ||
| return next(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not fail resolution on a malformed or non-readable ancestor package.json.
Only ENOENT is tolerated. A SyntaxError from JSON.parse inside readJson carries no code, so it reaches callback(err) and fails module creation. ENOTDIR and EISDIR behave the same way.
These ancestor files were never read before this change. A broken package.json in any ancestor directory inside compiler.context now breaks builds of unrelated modules. Treat an unreadable or unparsable ancestor file as "no flag here" and continue the walk.
🐛 Proposed fix
readJson(fs, join(fs, dir, "package.json"), (err, data) => {
if (err) {
- if ("code" in err && err.code === "ENOENT") {
- dir = parent;
- return next();
- }
- return callback(err);
+ // A broken ancestor package.json must not fail unrelated modules.
+ dir = parent;
+ return next();
}If you want to keep hard failures for genuine I/O faults, restrict the skip list explicitly to ENOENT, ENOTDIR, EISDIR, ENAMETOOLONG plus SyntaxError.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| readJson(fs, join(fs, dir, "package.json"), (err, data) => { | |
| if (err) { | |
| if ("code" in err && err.code === "ENOENT") { | |
| dir = parent; | |
| return next(); | |
| } | |
| return callback(err); | |
| } | |
| if (!data || typeof data !== "object" || Array.isArray(data)) { | |
| dir = parent; | |
| return next(); | |
| } | |
| readJson(fs, join(fs, dir, "package.json"), (err, data) => { | |
| if (err) { | |
| // A broken ancestor package.json must not fail unrelated modules. | |
| dir = parent; | |
| return next(); | |
| } | |
| if (!data || typeof data !== "object" || Array.isArray(data)) { | |
| dir = parent; | |
| return next(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/optimize/SideEffectsFlagPlugin.js` around lines 197 - 208, Update the
ancestor package.json error handling in the readJson callback to continue
walking via next() for missing or unreadable paths (ENOENT, ENOTDIR, EISDIR,
ENAMETOOLONG) and JSON parse SyntaxError, while preserving callback(err) for
other I/O failures. Keep the existing parent-directory traversal behavior
unchanged.
alexander-akait
left a comment
There was a problem hiding this comment.
That is interesting, initially we don’t inherit this specially, yes this idea was too, but as I remember we declined it in testing, let’s take a look how other bundlers do it
|
@alexander-akait Thanks. I checked other bundlers: Rspack/esbuild/Parcel/Rolldown all use the nearest enclosing package.json only (Rolldown explicitly declined walking up for a sideEffects field — rolldown#3336). I noticed this gap while reviewing production bundle output with AI: root sideEffects: false often gets shadowed by a nested type-only package.json under node_modules, which looks like either a packaging omission or an expectation that bundlers would inherit. This PR treats that as a reasonable optimization (scoped to node_modules, with context / named-package stops so app-local nests still shadow). If there was a clearer historical reason not to support this, we’re happy to reconsider. |
Summary
Nested type-only
package.jsonfiles (e.g.{"type":"module"}) were shadowing a parentsideEffectsflag, so unused modules from packages like entities stayed in the graph. Inherit the ancestor flag withincompiler.context, rebase globs to the declaring package root, and stop at named packages /node_modulesdirectory boundaries.What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Yes —
test/configCases/side-effects/nested-package-json-sideEffectsandnested-package-json-sideEffects-outside-context.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?
n/a
Use of AI
Yes — AI assisted implementation, tests, and PR drafting; changes were reviewed and verified locally.
Made with Cursor
Summary by CodeRabbit
Bug Fixes
sideEffectssettings in nested packages.Chores