feat(lazy-barrel): skip plain imports in side-effect-free modules#7986
feat(lazy-barrel): skip plain imports in side-effect-free modules#7986
Conversation
How to use the Graphite Merge QueueAdd the label graphite: merge-when-ready to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
shulaoda
force-pushed
the
01-21-feat_lazy-barrel_skip_plain_imports_in_side-effect-free_modules
branch
from
e9a8272 to
1f5ce1c
Compare
shulaoda
force-pushed
the
01-21-feat_lazy-barrel_skip_plain_imports_in_side-effect-free_modules
branch
from
1f5ce1c to
869aaeb
Compare
✅ Deploy Preview for rolldown-rs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR implements skipping of plain imports (like import './module', import {} from './module', and export {} from './module') in side-effect-free modules when lazy barrel optimization is enabled. Since barrel files must be marked as moduleSideEffects: false for lazy barrel optimization, these plain imports that don't contribute to exports can be safely skipped to further optimize build performance.
Changes:
- Added
IsPlainImportflag to track plain import statements that don't import any symbols - Updated AST scanner to mark plain imports during module scanning
- Modified module loader to skip loading plain imports in side-effect-free modules
- Added documentation explaining the plain import optimization behavior
- Added test case to verify plain imports are correctly skipped
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/rolldown_common/src/types/import_record.rs | Adds new IsPlainImport bitflag to identify plain import statements |
| crates/rolldown/src/ast_scanner/mod.rs | Sets IsPlainImport flag when scanning import declarations and export declarations with empty specifiers |
| crates/rolldown/src/module_loader/module_loader.rs | Implements logic to skip loading plain imports in side-effect-free modules during lazy barrel optimization |
| docs/in-depth/lazy-barrel-optimization.md | Documents the plain imports behavior, reorganizes sections, and updates configuration examples |
| packages/rolldown/tests/fixtures/lazy-barrel/plain-import/* | Adds comprehensive test case verifying plain imports are skipped in side-effect-free barrel modules |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Benchmarks Rust
|
| export {} from './module'; // empty re-export | ||
| ``` | ||
|
|
||
| In **side-effect-free** modules, these plain imports are skipped. Otherwise, they are loaded normally. |
There was a problem hiding this comment.
Is the description proper? ModuleA's not having side effect doesn't mean its dependency don't have side effect.
There was a problem hiding this comment.
Just be sure our behavior is aligned with other bundlers.
There was a problem hiding this comment.
import './module'; // side-effect import import {} from './module'; // empty import export {} from './module'; // empty re-export
Rspack will eagerly load the first two cases by default, but it will eliminate the last one. Since
export {} from '..'is invalid syntax and very uncommon, I didn’t handle it and treated it asimport '..'instead.
That said, your point also makes sense. We don’t necessarily need to match Rspack’s behavior, but I need think through whether removing these could introduce any potential issues. 🤔
Rollup works like:
See #7969 (comment)
There was a problem hiding this comment.
Let me think this through again — I have a feeling something’s off.
There was a problem hiding this comment.
It’s not just Rspack — I should also look at how Turbopack and Parcel handle the constraints and behavior around Lazy Barrel. I’ll share my conclusions afterward.
There was a problem hiding this comment.
I’ve done some research, and aside from Rspack’s behavior mentioned earlier, Parcel’s behavior is consistent with our new implementation. For side-effect-free modules, import './module', import {} from './module', and export {} from './module' are not eagerly loaded. Parcel also allows configuring sideEffects at both the module and package level.
Turbopack follows a similar behavior, although it only supports configuring sideEffects at the package level.
Based on this, I believe our new implementation is correct. We need to fully trust the user’s explicit side-effect-free declarations; otherwise, Lazy Barrel cannot be implemented in a sound and meaningful way.
^cc @sapphi-red
There was a problem hiding this comment.
It seems I was misunderstanding what "marking a module as sideeffect" means.
#8005 (comment)
Co-authored-by: Copilot <[email protected]> Signed-off-by: dalaoshu <[email protected]>
sapphi-red
force-pushed
the
01-21-feat_lazy-barrel_skip_plain_imports_in_side-effect-free_modules
branch
from
b589854 to
40dd19a
Compare
3 participants
Since we require the barrel file to be marked with
moduleSideEffects: false, I guess we should remove these imports to be consistent.Originally posted by @sapphi-red in #7969 (comment)