fix(ConcatenatedModule): include runtimeCondition of external infos in updateHash#21023
Merged
Conversation
…Condition hashing
🦋 Changeset detectedLatest commit: 4ab1bf8 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 |
Contributor
|
This PR is packaged and the instant preview is available (75f60f6). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@75f60f6
yarn add -D webpack@https://pkg.pr.new/webpack@75f60f6
pnpm add -D webpack@https://pkg.pr.new/webpack@75f60f6 |
alexander-akait
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #21023 +/- ##
==========================================
+ Coverage 91.55% 91.58% +0.02%
==========================================
Files 573 573
Lines 59502 59541 +39
Branches 16069 16077 +8
==========================================
+ Hits 54479 54531 +52
+ Misses 5023 5010 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes persistent-cache invalidation for ConcatenatedModule by ensuring runtime-conditional state for concatenated external infos contributes to the module hash, addressing a long-standing TODO in ConcatenatedModule#updateHash. It also adds a regression test case and includes a small comment correction in the JS parser.
Changes:
- Include
runtimeConditionforinfo.type === "external"entries inConcatenatedModule#updateHash. - Add a
configCasesregression test that asserts the concatenated module hash changes when only an external-inforuntimeConditionchanges. - Update a TODO comment in
JavascriptParserto reference import phases (not assertions) and add a changeset entry.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
lib/optimize/ConcatenatedModule.js |
Hash now incorporates external-info runtimeCondition so runtime-conditional externals affect cache invalidation. |
test/configCases/concatenate-modules/external-runtime-condition-hash/webpack.config.js |
New config-case plugin asserts hash differs when external runtimeCondition differs. |
test/configCases/concatenate-modules/external-runtime-condition-hash/index.js |
Test entry verifies compilation works with an external excluded from concatenation. |
test/configCases/concatenate-modules/external-runtime-condition-hash/lib.js |
Creates an external import/re-export to force an external info entry in concatenation list. |
test/configCases/concatenate-modules/external-runtime-condition-hash/errors.js |
Declares no expected errors for the new config-case. |
lib/javascript/JavascriptParser.js |
Comment-only TODO clarification (import phases). |
.changeset/concatenated-module-hash-runtime-condition.md |
Records the patch-level change for release notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
2319
to
2322
| case "external": | ||
| hash.update(`${chunkGraph.getModuleId(info.module)}`); | ||
| // TODO runtimeCondition | ||
| hash.update(runtimeConditionToString(info.runtimeCondition)); | ||
| break; |
Member
Author
There was a problem hiding this comment.
Good catch — fixed in ba077ce by combining moduleId and runtimeCondition into a single hash.update with a | delimiter, so the two segments can no longer slide across each other in the streaming hash.
Generated by Claude Code
…ks in test plugin
…h to avoid collisions
Comment on lines
2319
to
+2322
| case "external": | ||
| hash.update(`${chunkGraph.getModuleId(info.module)}`); | ||
| // TODO runtimeCondition | ||
| hash.update( | ||
| `${chunkGraph.getModuleId(info.module)}|${runtimeConditionToString(info.runtimeCondition)}` | ||
| ); |
Member
Author
There was a problem hiding this comment.
Good point — verified both fields actually drive code generation:
info.deferred(set frommoduleGraph.isDeferred(info.module)) gates the deferred-external branch inrenderConcatenatedModule(if (info.type === "external" && info.deferred),if (!info.deferred))info.nonDeferAccesstogether withinfo.deferredtriggers the extra non-defer-access path (if (info.deferred && nonDeferAccess))
Both can change without the module id changing (e.g. when a different consumer adds/removes an import.defer), so the same persistent-cache staleness applies. Folded both into the hash update in 4ab1bf8 — same single hash.update with |-delimited segments.
Generated by Claude Code
Contributor
Types CoverageCoverage after merging claude/easy-todo-fixes-TRVLy into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the long-standing
// TODO runtimeConditioninConcatenatedModule#updateHash(introduced with the original concatenation work). For aninfo.type === "external"entry, the hash previously only fed in the external module's id and dropped itsruntimeCondition. Two ConcatenatedModule states that differ only in the runtime condition of a concatenated external therefore produced identical hashes, which can slip past persistent-cache invalidation.This PR feeds
runtimeConditionToString(info.runtimeCondition)into the hash alongside the module id, so the condition contributes to the digest the same way it already does for other parts of the graph.Also includes a small unrelated comment fix on
lib/javascript/JavascriptParser.js: theImportExpressioncast was tagged with a TODO mentioning "import assertions", but the cast actually adds thephaseproperty (defer/source). Renamed to "import phases".What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Yes —
test/configCases/concatenate-modules/external-runtime-condition-hash/. The case builds a real UMD-external project (UMD externals bail out of concatenation viaExternalModule#getConcatenationBailoutReason, so a realinfo.type === "external"entry appears in the concat list), then asserts in a plugin that swapping onlyruntimeConditionon the otherwise-real list produces a different digest. Verified the case fails on the unmodified baseline ("More errors (1 instead of 0) ... Expected: not ") and passes with the fix, under bothConfigTestCases.basictest.jsandConfigCacheTestCases.basictest.js.Does this PR introduce a breaking change?
No. The hash now incorporates additional state; module hashes for concatenations involving a non-concatenatable external will change once, then stay stable.
If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a
Use of AI
Claude Code was used under human review to locate the TODO, implement the fix, and author the configCases regression test. All changes were reviewed and the regression was double-checked by reverting the fix locally and confirming the new test fails with a hash-mismatch error before re-applying.