Skip to content

fix: Fix same-target common chunk merge cycle check#9329

Closed
schiller-manuel wants to merge 2 commits into
rolldown:mainfrom
schiller-manuel:fix-chunk-merge
Closed

fix: Fix same-target common chunk merge cycle check#9329
schiller-manuel wants to merge 2 commits into
rolldown:mainfrom
schiller-manuel:fix-chunk-merge

Conversation

@schiller-manuel

@schiller-manuel schiller-manuel commented May 8, 2026 •
edited
Loading

Copy link
Copy Markdown

Fix common chunk merge false positive for same-target merges

Fixes a chunk optimizer case where multiple pending common chunks that should all merge into the same entry could block each other via a transient circular-dependency check.

This showed up with React's CJS react/jsx-runtime facade:

module.exports = require('./cjs/react-jsx-runtime.production.js')

In a graph with one user entry and nested dynamic imports, Rolldown could emit a separate jsx-runtime common chunk even though all consumers are reachable through the same entry. The optimizer now treats other valid same-target pending merges as internal to the target when checking for cycles.

created using GPT 5.5

@codspeed-hq

codspeed-hq Bot commented May 8, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 4 untouched benchmarks
⏩ 10 skipped benchmarks1

Comparing schiller-manuel:fix-chunk-merge (8777002) with main (d69e071)2

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. ↩

  2. No successful run was found on main (d3ae8ba) during the generation of this report, so d69e071 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

@IWANABETHATGUY

IWANABETHATGUY commented May 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Please file an issue first

@IWANABETHATGUY IWANABETHATGUY marked this pull request as draft May 9, 2026 04:05
@IWANABETHATGUY

IWANABETHATGUY commented May 9, 2026

Copy link
Copy Markdown
Member

Thanks for your contribution.
The issue is already fixed in #9305 prefer 9305 since it is a more general solution rather than patching the existing code.

@IWANABETHATGUY IWANABETHATGUY mentioned this pull request May 9, 2026
Closed
@schiller-manuel

schiller-manuel commented May 9, 2026

Copy link
Copy Markdown
Author

@IWANABETHATGUY thanks for working on this.

BTW I read the contribution guideline at https://rolldown.rs/contribution-guide/#submitting-a-pull-request before creating the PR, it wasn't obvious to me that I should have created an issue first.

@IWANABETHATGUY

IWANABETHATGUY commented May 9, 2026

Copy link
Copy Markdown
Member

@IWANABETHATGUY thanks for working on this.

BTW I read the contribution guideline at https://rolldown.rs/contribution-guide/#submitting-a-pull-request before creating the PR, it wasn't obvious to me that I should have created an issue first.

Yeah, you are right. Creating an issue and discussing it with a team member can help avoid missing some important context, which may improve the AI-generated code. Sometimes there are multiple ways to solve the same issue, and AI usually chooses the easiest, but it may not be the correct one.

@schiller-manuel

schiller-manuel commented May 9, 2026

Copy link
Copy Markdown
Author

will do in future!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

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

@schiller-manuel @IWANABETHATGUY