Skip to content

Conversation

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Sep 21, 2025

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

This very much simplifies the logic to prevent merges with top-level-await modules that create deadlocks. The proposed logic is to just assume that a file that contains a top-level await will await all dynamic imports within the module. This will prevent that any dependencies of dynamically imported modules are merged into the same chunk as the current module.

cc @TrickyPi, what do you think?

There are different scenarios where this can lead to cycles. This pessimistic
approach should solve all of them.
@vercel
Copy link

vercel bot commented Sep 21, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rollup Ready Ready Preview Comment Sep 21, 2025 6:09pm

@github-actions
Copy link

github-actions bot commented Sep 21, 2025

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#improve-tla-deadlock-detection

Notice: Ensure you have installed the latest nightly Rust toolchain. If you haven't installed it yet, please see https://www.rust-lang.org/tools/install to learn how to download Rustup and install Rust.

or load it into the REPL:
https://rollup-f3tn119vl-rollup-js.vercel.app/repl/?pr=6121

@github-actions
Copy link

Performance report

  • BUILD: 7136ms, 815 MB
    • initialize: 0ms, 25 MB
    • generate module graph: 2660ms, 628 MB
      • generate ast: 1388ms (-46ms, -3.2%), 622 MB
    • sort and bind modules: 403ms, 679 MB
    • mark included statements: 4067ms, 815 MB
      • treeshaking pass 1: 2401ms, 814 MB
      • treeshaking pass 2: 471ms, 836 MB
      • treeshaking pass 3: 404ms, 817 MB
      • treeshaking pass 4: 390ms, 841 MB
      • treeshaking pass 5: 388ms, 815 MB
  • GENERATE: 1028ms, 914 MB
    • initialize render: 0ms, 938 MB (+3%)
    • generate chunks: 256ms (+20ms, +8.5%), 890 MB (+7%)
      • optimize chunks: 0ms, 921 MB (+12%)
    • render chunks: 741ms, 900 MB
    • transform chunks: 20ms, 914 MB
    • generate bundle: 0ms, 914 MB

@codecov
Copy link

codecov bot commented Sep 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.73%. Comparing base (2029f63) to head (5c0adf7).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6121      +/-   ##
==========================================
- Coverage   98.74%   98.73%   -0.01%     
==========================================
  Files         271      271              
  Lines       10644    10630      -14     
  Branches     2850     2846       -4     
==========================================
- Hits        10510    10496      -14     
  Misses         89       89              
  Partials       45       45              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@TrickyPi
Copy link
Member

TrickyPi commented Sep 22, 2025

Looks good to me! That’s a really nice and simple approach. Although other non-TLA dynamic imports will also be considered as TLA, it completely eliminates this kind of issue.

@lukastaegert
Copy link
Member Author

I mean the alternative would be to either force comments upon people who want stuff to "just work", or to lift analysis on a completely different level. My hope is that top-level await is not that common anyway. And in the worst case, it will just be like just turning off the dynamic import optimization, i.e. it will be like most other bundlers.

@lukastaegert lukastaegert merged commit 5eee44c into master Sep 23, 2025
46 checks passed
@lukastaegert lukastaegert deleted the improve-tla-deadlock-detection branch September 23, 2025 04:06
@github-actions
Copy link

This PR has been released as part of [email protected]. You can test it via npm install rollup.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rollup creates circular dependencies with dynamic imports + top-level await

3 participants