8000 consider TLA imports have higher execution priority by TrickyPi · Pull Request #5937 · rollup/rollup · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

consider TLA imports have higher execution priority #5937

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 28, 2025
Merged

Conversation

TrickyPi
Copy link
Member

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:
resolves #5552

Description

This PR moves the detection of top-level await imports from the include phase to the initializer phase. This allows us to set the execIndex of imported modules to be executed before their importing modules during module sorting.

This change resolves most scenarios where inlineDynamicImports and TLA imports coexist. However, there is one edge case that remains problematic:

// main.js
window.foo = 1;

await import('./lib.js');

// lib.js
window.foo = 2;

In this scenario, both main.js and lib.js modify the window.foo property. Before bundling, the final value of foo would be 2. However, after bundling, the final value becomes 1.

Despite this edge case, this PR successfully addresses the majority of current error scenarios.

Copy link
vercel bot commented Apr 25, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
rollup ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 25, 2025 4:06pm

@TrickyPi TrickyPi requested a review from Copilot April 25, 2025 15:41
Copy link
@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR improves module execution ordering by moving top-level await (TLA) import detection from the include phase to the initializer phase. It updates both the AST node handling for dynamic imports and the expected outputs to better support inline dynamic imports with TLA, and refines the cycle dependency test configuration.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/form/samples/no-treeshake/_expected.js Updated expected output for dynamic imports with TLA adjustments
test/form/samples/cycles-dependency-with-TLA-await-import/main.js Added a sample demonstrating a TLA import in a cycle dependency scenario
test/form/samples/cycles-dependency-with-TLA-await-import/lib.js Added a sample lib module for TLA and cycle dependency testing
test/form/samples/cycles-dependency-with-TLA-await-import/_expected.js Updated expected output for cycle dependency scenario with TLA support
test/form/samples/cycles-dependency-with-TLA-await-import/_config.js Added configuration to expect a circular dependency warning
src/utils/executionOrder.ts Refactored module execution order handling to integrate TLA detection
src/ast/nodes/ImportExpression.ts Updated include and initialization methods to compute TLA status without context args
src/ast/nodes/AwaitExpression.ts Removed the explicit top-level await flag handling in favor of initializer logic
src/ast/ExecutionContext.ts Removed the withinTopLevelAwait flag from the inclusion context interface
Comments suppressed due to low confidence (1)

src/ast/nodes/ImportExpression.ts:244

  • ArrowFunctionExpression is used in the TLA detection block but is not imported in this file; please import it from './ArrowFunctionExpression' to avoid a reference error.
if (withinAwaitExpression && (parent instanceof FunctionNode || parent instanceof ArrowFunctionExpression)) {

Copy link
github-actions bot commented Apr 25, 2025

Thank you for your contribution! ❤️

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

npm install rollup/rollup#fix/5552

Notice: Ensure you have installed the latest stable 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-mir91y515-rollup-js.vercel.app/repl/?pr=5937

Copy link
github-actions bot commented Apr 25, 2025

Performance report

  • BUILD: 7510ms, 745 MB
    • initialize: 0ms, 28.2 MB
    • generate module graph: 2862ms, 561 MB
      • generate ast: 1274ms, 554 MB
    • sort and bind modules: 418ms, 605 MB
    • mark included statements: 4231ms, 745 MB
      • treeshaking pass 1: 2483ms, 744 MB
      • treeshaking pass 2: 492ms, 744 MB
      • treeshaking pass 3: 424ms, 745 MB
      • treeshaking pass 4: 418ms (+12ms, +2.9%), 750 MB
      • treeshaking pass 5: 407ms, 745 MB
  • GENERATE: 763ms, 986 MB
    • initialize render: 0ms, 883 MB
    • generate chunks: 73ms, 895 MB
      • optimize chunks: 0ms, 888 MB
    • render chunks: 668ms, 964 MB
    • transform chunks: 19ms, 986 MB
    • generate bundle: 0ms, 986 MB

Copy link
codecov bot commented Apr 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.55%. Comparing base (7fc5036) to head (ce78c96).
Report is 6 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5937   +/-   ##
=======================================
  Coverage   98.55%   98.55%           
=======================================
  Files         270      270           
  Lines        8692     8701    +9     
  Branches     1488     1490    +2     
=======================================
+ Hits         8566     8575    +9     
  Misses         93       93           
  Partials       33       33           

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

@vercel vercel < 8000 span class="Label Label--secondary">bot deployed to Preview April 25, 2025 16:06 View deployment
Copy link
Member
@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is a good idea to treat TLA modules like synchronous imports. While there are still edge cases, this is definitely a net improvement.

@lukastaegert lukastaegert added this pull request to the merge queue Apr 28, 2025
Merged via the queue into master with commit cb8f815 Apr 28, 2025
42 checks passed
@lukastaegert lukastaegert deleted the fix/5552 branch April 28, 2025 04:43
Copy link
github-actions bot commented May 6, 2025

This PR has been released as part of rollup@4.40.2. 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.

inlineDynamicImports with top level await import() cause runtime error
2 participants
0