Skip to content

fix: preserve included external import declarations#10009

Closed
chuganzy wants to merge 1 commit into
rolldown:mainfrom
chuganzy:fix-strict-execution-order
Closed

fix: preserve included external import declarations#10009
chuganzy wants to merge 1 commit into
rolldown:mainfrom
chuganzy:fix-strict-execution-order

Conversation

@chuganzy

@chuganzy chuganzy commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

This PR aims to fix a case where the finalizer removed an included external import declaration.

The finalizer handles both import declarations and export ... from declarations.
Previously, when an import record did not resolve to an internal module, the finalizer removed the statement. That is not safe for an included external import declaration: the imported bindings may still be referenced by code that survived tree-shaking.

This showed up with strictExecutionOrder because module bodies can be moved into __esmMin wrappers. The wrapper body could still contain code like:

value = first(second);

while the corresponding top-level import had been removed:

import { first, second } from "dep";

This keeps included unresolved/external ImportDeclarations in the output, while leaving unresolved export ... from handling unchanged.

@chuganzy chuganzy marked this pull request as draft June 29, 2026 11:38
@codspeed-hq

codspeed-hq Bot commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 7 untouched benchmarks
⏩ 10 skipped benchmarks1

Comparing chuganzy:fix-strict-execution-order (d93e068) with main (520d81e)

Open in CodSpeed

Footnotes

  1. 10 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@chuganzy chuganzy force-pushed the fix-strict-execution-order branch from 1b44a53 to d93e068 Compare June 29, 2026 11:42
@chuganzy chuganzy marked this pull request as ready for review June 29, 2026 11:43
@IWANABETHATGUY

IWANABETHATGUY commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Could you please file an issue with a minimal reproduction first? I’m not sure what you’re trying to fix. We’ve updated our CONTRIBUTING.md, https://github.com/rolldown/rolldown/blob/main/docs/contribution-guide/index.md, and in the future we may close such PRs.

@IWANABETHATGUY IWANABETHATGUY marked this pull request as draft June 29, 2026 11:47
@chuganzy chuganzy mentioned this pull request Jun 29, 2026
Closed
@chuganzy

chuganzy commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@IWANABETHATGUY Apologies for not creating the issue first and for not providing enough context in the PR description.

I filed a minimal issue here: #10013

The original failure was found while bundling Storybook. I used Codex to help reduce that failure into a minimal reproduction, but I reviewed and adjusted the test case and fix by hand.

The reduced case shows that with strictExecutionOrder: true, Rolldown can keep a lazy __esmMin wrapper that still references external bindings while dropping the corresponding ImportDeclaration, leaving free identifiers in the generated chunk.

@chuganzy chuganzy marked this pull request as ready for review June 29, 2026 12:09
@chuganzy chuganzy deleted the fix-strict-execution-order branch July 3, 2026 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@IWANABETHATGUY IWANABETHATGUY Awaiting requested review from IWANABETHATGUY IWANABETHATGUY is a code owner
@shulaoda shulaoda Awaiting requested review from shulaoda shulaoda is a code owner
@hyf0 hyf0 Awaiting requested review from hyf0 hyf0 is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@chuganzy @IWANABETHATGUY