-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this 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)) {
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: |
Performance report
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. 🚀 New features to boost your workflow:
|
There was a problem hiding this 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.
This PR has been released as part of rollup@4.40.2. You can test it via |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
resolves #5552
Description
This PR moves the detection of top-level await imports from the
include
phase to theinitializer
phase. This allows us to set theexecIndex
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:In this scenario, both
main.js
andlib.js
modify thewindow.foo
property. Before bundling, the final value offoo
would be2
. However, after bundling, the final value becomes1
.Despite this edge case, this PR successfully addresses the majority of current error scenarios.