fix: link import bindings inside define callbacks (#17063)#20990
Merged
Conversation
🦋 Changeset detectedLatest commit: 95eee93 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 (eb14a5a). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@eb14a5a
yarn add -D webpack@https://pkg.pr.new/webpack@eb14a5a
pnpm add -D webpack@https://pkg.pr.new/webpack@eb14a5a |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20990 +/- ##
=======================================
Coverage 90.94% 90.94%
=======================================
Files 573 573
Lines 58940 58947 +7
Branches 15888 15892 +4
=======================================
+ Hits 53601 53611 +10
+ Misses 5339 5336 -3
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 a parsing edge case where, in Harmony/ESM modules, HarmonyDetectionParserPlugin short-circuited define(...) calls and inadvertently skipped walking their arguments. That prevented imported bindings referenced inside inline AMD factory callbacks (or via callback identifiers passed to define) from being tracked, which could lead to incorrect tree-shaking and even runtime ReferenceErrors.
Changes:
- Update
HarmonyDetectionParserPluginto walkdefine(...)/exports(...)call arguments in Harmony modules before skipping default AMD/CommonJS handling. - Add a parsing test case to ensure default/named imports used inside
definecallbacks are correctly linked. - Add an inner-graph configCase to ensure tree-shaking keeps the correct imported bindings when
definecallbacks reference them.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
lib/dependencies/HarmonyDetectionParserPlugin.js |
Walks define/exports call arguments in Harmony modules to ensure import binding references inside callbacks are discovered. |
test/cases/parsing/issue-17063/index.js |
Regression test validating import bindings are linked when referenced inside define(...) callbacks. |
test/cases/parsing/issue-17063/lib.js |
Provides default and named exports used by the parsing regression test. |
test/configCases/inner-graph/issue-17063/module.js |
Inner-graph regression reproducer for tree-shaking of imports used inside define callbacks / passed callbacks. |
test/configCases/inner-graph/issue-17063/webpack.config.js |
Adds the inner-graph regression assertions for used exports. |
.changeset/fix-import-binding-in-define.md |
Patch changeset describing the fix and affected patterns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alexander-akait
force-pushed
the
claude/fix-issue-17063-e3dSi
branch
2 times, most recently
from
24b1c66 to
9d98a86
Compare
`HarmonyDetectionParserPlugin` short-circuited `hooks.call.for("define")`
in harmony modules, which skipped walking the arguments of `define(...)`
calls. As a result, references to imported bindings inside an inline AMD
factory (e.g. `define(function () { console.log(foo); })`) were never
rewritten, causing `ReferenceError` at runtime. The same omission also
hid usages from inner graph, so patterns like `const fn = () => foo;
define(fn);` would tree-shake `foo` away.
Walk the call arguments before skipping the default AMD/CommonJS
handling so the harmony parser sees those references.
alexander-akait
force-pushed
the
claude/fix-issue-17063-e3dSi
branch
from
9d98a86 to
95eee93
Compare
Contributor
Types CoverageCoverage after merging claude/fix-issue-17063-e3dSi 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.
HarmonyDetectionParserPluginshort-circuitedhooks.call.for("define")in harmony modules, which skipped walking the arguments of
define(...)calls. As a result, references to imported bindings inside an inline AMD
factory (e.g.
define(function () { console.log(foo); })) were neverrewritten, causing
ReferenceErrorat runtime. The same omission alsohid usages from inner graph, so patterns like
const fn = () => foo; define(fn);would tree-shakefooaway.Walk the call arguments before skipping the default AMD/CommonJS
handling so the harmony parser sees those references.
fixes #17063
fixes #20542