fix: skip dropping side-effects on namespaceReexportsByName cache hit (#6274)#6286
Conversation
|
@littlegrayss is attempting to deploy a commit to the rollup-js Team on Vercel. A member of the Team first needs to authorize it. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6286 +/- ##
=======================================
Coverage 98.78% 98.78%
=======================================
Files 274 274
Lines 10772 10788 +16
Branches 2878 2881 +3
=======================================
+ Hits 10641 10657 +16
Misses 89 89
Partials 42 42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…rollup#6274) When multiple entries reference the same variable via a namespace re-export, the `namespaceReexportsByName` cache was returning early. This prevented `getVariableFromNamespaceReexports` from capturing the side-effect dependencies for the subsequent importers, leading to dropped side effects (such as CSS imports). This fix explicitly re-invokes `getVariableFromNamespaceReexports` when the cache is hit and an `importerForSideEffects` is provided, utilizing its traversal to correctly link side effects to the new importer, while ignoring the return value. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
…ceReexportsByName cache hit Replace the re-traversal approach with a parallel `namespaceSideEffectsByName` cache that stores the collected side-effect modules and cyclic reexporters on first resolution. Cache hits now replay the stored metadata for each importer directly, avoiding redundant graph traversal while preserving correct side-effect tracking across multiple importers. Update the test to cover the multi-entry scenario where two entries resolve the same export through the namespace reexport cache.
c92d7df to
5154ad7
Compare
|
Thanks for the update. I simplified the implementation and added the missing tests on your branch. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Fixes a Rollup module graph bug where namespaceReexportsByName cache hits for export * could skip registering side-effect dependencies (and related cyclic reexport handling) for subsequent importers, leading to dropped side effects in bundled output.
Changes:
- Extends
Module.getVariableForExportName/getVariableFromNamespaceReexportsto cache and replay side-effect and reexport-chain metadata onexport *cache hits. - Adds a chunking-form regression test to ensure side-effectful modules are preserved for multiple entry points across formats.
- Adds a function test covering cyclic reexport scenarios with multiple importers when namespace reexport caching is involved.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Module.ts | Caches and replays side-effect + cyclic reexport metadata for export * namespace reexport cache hits. |
| test/function/samples/circular-namespace-reexport-cache/_config.js | Asserts warnings for cyclic reexports when multiple importers hit the namespace reexport cache. |
| test/function/samples/circular-namespace-reexport-cache/main.js | Sample entry aggregating two importers to exercise cyclic graph behavior. |
| test/function/samples/circular-namespace-reexport-cache/entry1.js | Sample importer #1 for the cyclic namespace reexport scenario. |
| test/function/samples/circular-namespace-reexport-cache/entry2.js | Sample importer #2 for the cyclic namespace reexport scenario. |
| test/function/samples/circular-namespace-reexport-cache/lib/index.js | Barrel module using export * to trigger namespace reexport caching. |
| test/function/samples/circular-namespace-reexport-cache/lib/foo.js | Reexport + side-effect import to participate in cyclic chain. |
| test/function/samples/circular-namespace-reexport-cache/lib/fooImpl.js | Implementation module that creates cycles by importing entry modules. |
| test/function/samples/circular-namespace-reexport-cache/lib/effect.js | Side-effect module used by the cyclic scenario. |
| test/chunking-form/samples/namespace-reexport-side-effect-cache/_config.js | Configures treeshake side-effects to reproduce/guard against the cache-hit side-effect drop. |
| test/chunking-form/samples/namespace-reexport-side-effect-cache/entry1.js | Entry point #1 importing through export * barrel. |
| test/chunking-form/samples/namespace-reexport-side-effect-cache/entry2.js | Entry point #2 importing through export * barrel (regression target). |
| test/chunking-form/samples/namespace-reexport-side-effect-cache/lib/index.js | Barrel module using export * to drive namespace reexport caching. |
| test/chunking-form/samples/namespace-reexport-side-effect-cache/lib/foo.js | Reexport + side-effect import module (should be retained for both entries). |
| test/chunking-form/samples/namespace-reexport-side-effect-cache/lib/fooImpl.js | Export implementation for the chunking-form regression test. |
| test/chunking-form/samples/namespace-reexport-side-effect-cache/lib/effect.js | Side-effect module that must remain reachable from both entries. |
| test/chunking-form/samples/namespace-reexport-side-effect-cache/_expected/es/generated-effect.js | Expected ES output for shared side-effect chunk. |
| test/chunking-form/samples/namespace-reexport-side-effect-cache/_expected/es/entry1.js | Expected ES entry1 to depend on side-effect chunk. |
| test/chunking-form/samples/namespace-reexport-side-effect-cache/_expected/es/entry2.js | Expected ES entry2 to depend on side-effect chunk. |
| test/chunking-form/samples/namespace-reexport-side-effect-cache/_expected/cjs/generated-effect.js | Expected CJS output for shared side-effect chunk. |
| test/chunking-form/samples/namespace-reexport-side-effect-cache/_expected/cjs/entry1.js | Expected CJS entry1 to require side-effect chunk. |
| test/chunking-form/samples/namespace-reexport-side-effect-cache/_expected/cjs/entry2.js | Expected CJS entry2 to require side-effect chunk. |
| test/chunking-form/samples/namespace-reexport-side-effect-cache/_expected/amd/generated-effect.js | Expected AMD output for shared side-effect chunk. |
| test/chunking-form/samples/namespace-reexport-side-effect-cache/_expected/amd/entry1.js | Expected AMD entry1 to depend on side-effect chunk. |
| test/chunking-form/samples/namespace-reexport-side-effect-cache/_expected/amd/entry2.js | Expected AMD entry2 to depend on side-effect chunk. |
| test/chunking-form/samples/namespace-reexport-side-effect-cache/_expected/system/generated-effect.js | Expected System output for shared side-effect chunk. |
| test/chunking-form/samples/namespace-reexport-side-effect-cache/_expected/system/entry1.js | Expected System entry1 to depend on side-effect chunk. |
| test/chunking-form/samples/namespace-reexport-side-effect-cache/_expected/system/entry2.js | Expected System entry2 to depend on side-effect chunk. |
|
This PR has been released as part of rollup@4.60.1. You can test it via |
This PR contains the following updates: | Package | Change | [Age](https://docs.renovatebot.com/merge-confidence/) | [Confidence](https://docs.renovatebot.com/merge-confidence/) | |---|---|---|---| | [@eslint/compat](https://github.com/eslint/rewrite/tree/main/packages/compat#readme) ([source](https://github.com/eslint/rewrite/tree/HEAD/packages/compat)) | [`2.0.3` → `2.0.4`](https://renovatebot.com/diffs/npm/@eslint%2fcompat/2.0.3/2.0.4) |  |  | | [@rollup/rollup-linux-x64-gnu](https://rollupjs.org/) ([source](https://github.com/rollup/rollup)) | [`4.60.0` → `4.60.1`](https://renovatebot.com/diffs/npm/@rollup%2frollup-linux-x64-gnu/4.60.0/4.60.1) |  |  | | [@types/node](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/node) ([source](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node)) | [`24.12.0` → `24.12.2`](https://renovatebot.com/diffs/npm/@types%2fnode/24.12.0/24.12.2) |  |  | | [@typescript-eslint/eslint-plugin](https://typescript-eslint.io/packages/eslint-plugin) ([source](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin)) | [`8.57.2` → `8.58.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/8.57.2/8.58.0) |  |  | | [@typescript-eslint/parser](https://typescript-eslint.io/packages/parser) ([source](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser)) | [`8.57.2` → `8.58.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/8.57.2/8.58.0) |  |  | | [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | [`10.1.0` → `10.2.0`](https://renovatebot.com/diffs/npm/eslint/10.1.0/10.2.0) |  |  | | [rollup](https://rollupjs.org/) ([source](https://github.com/rollup/rollup)) | [`4.60.0` → `4.60.1`](https://renovatebot.com/diffs/npm/rollup/4.60.0/4.60.1) |  |  | | [ts-jest](https://kulshekhar.github.io/ts-jest) ([source](https://github.com/kulshekhar/ts-jest)) | [`29.4.6` → `29.4.9`](https://renovatebot.com/diffs/npm/ts-jest/29.4.6/29.4.9) |  |  | --- ### Release Notes <details> <summary>eslint/rewrite (@​eslint/compat)</summary> ### [`v2.0.4`](https://github.com/eslint/rewrite/blob/HEAD/packages/compat/CHANGELOG.md#204-2026-04-03) [Compare Source](eslint/rewrite@41eb19f...fe114ee) ##### Dependencies - The following workspace dependencies were updated - dependencies - [@​eslint/core](https://github.com/eslint/core) bumped from ^1.1.1 to ^1.2.0 </details> <details> <summary>rollup/rollup (@​rollup/rollup-linux-x64-gnu)</summary> ### [`v4.60.1`](https://github.com/rollup/rollup/blob/HEAD/CHANGELOG.md#4601) [Compare Source](rollup/rollup@v4.60.0...v4.60.1) *2026-03-30* ##### Bug Fixes - Resolve a situation where side effect imports could be dropped due to a caching issue ([#​6286](rollup/rollup#6286)) ##### Pull Requests - [#​6286](rollup/rollup#6286): fix: skip dropping side-effects on namespaceReexportsByName cache hit ([#​6274](rollup/rollup#6274)) ([@​littlegrayss](https://github.com/littlegrayss), [@​TrickyPi](https://github.com/TrickyPi)) - [#​6317](rollup/rollup#6317): chore(deps): pin dependencies ([@​renovate](https://github.com/renovate)\[bot], [@​lukastaegert](https://github.com/lukastaegert)) - [#​6318](rollup/rollup#6318): chore(deps): update msys2/setup-msys2 digest to [`cafece8`](rollup/rollup@cafece8) ([@​renovate](https://github.com/renovate)\[bot], [@​lukastaegert](https://github.com/lukastaegert)) - [#​6319](rollup/rollup#6319): chore(deps): update minor/patch updates ([@​renovate](https://github.com/renovate)\[bot], [@​lukastaegert](https://github.com/lukastaegert)) - [#​6320](rollup/rollup#6320): chore(deps): pin dependency typescript to v5 ([@​renovate](https://github.com/renovate)\[bot], [@​lukastaegert](https://github.com/lukastaegert)) - [#​6321](rollup/rollup#6321): chore(deps): update openharmony-rs/setup-ohos-sdk action to v1 ([@​renovate](https://github.com/renovate)\[bot], [@​lukastaegert](https://github.com/lukastaegert)) - [#​6322](rollup/rollup#6322): fix(deps): update swc monorepo (major) ([@​renovate](https://github.com/renovate)\[bot], [@​lukastaegert](https://github.com/lukastaegert)) - [#​6323](rollup/rollup#6323): chore(deps): lock file maintenance ([@​renovate](https://github.com/renovate)\[bot]) - [#​6324](rollup/rollup#6324): chore(deps): lock file maintenance ([@​renovate](https://github.com/renovate)\[bot], [@​lukastaegert](https://github.com/lukastaegert)) </details> <details> <summary>typescript-eslint/typescript-eslint (@​typescript-eslint/eslint-plugin)</summary> ### [`v8.58.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8580-2026-03-30) [Compare Source](typescript-eslint/typescript-eslint@v8.57.2...v8.58.0) ##### 🚀 Features - support TypeScript 6 ([#​12124](typescript-eslint/typescript-eslint#12124)) ##### 🩹 Fixes - **eslint-plugin:** \[prefer-regexp-exec] avoid fixing unknown RegExp flags ([#​12161](typescript-eslint/typescript-eslint#12161)) - **eslint-plugin:** \[no-extraneous-class] handle index signatures ([#​12142](typescript-eslint/typescript-eslint#12142)) - **eslint-plugin:** crash in `no-unnecessary-type-arguments` ([#​12163](typescript-eslint/typescript-eslint#12163)) ##### ❤️ Thank You - ej shafran [@​ej-shafran](https://github.com/ej-shafran) - Evyatar Daud [@​StyleShit](https://github.com/StyleShit) - GG ZIBLAKING - milkboy2564 [@​SeolJaeHyeok](https://github.com/SeolJaeHyeok) - teee32 [@​teee32](https://github.com/teee32) See [GitHub Releases](https://github.com/typescript-eslint/typescript-eslint/releases/tag/v8.58.0) for more information. You can read about our [versioning strategy](https://typescript-eslint.io/users/versioning) and [releases](https://typescript-eslint.io/users/releases) on our website. </details> <details> <summary>typescript-eslint/typescript-eslint (@​typescript-eslint/parser)</summary> ### [`v8.58.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#8580-2026-03-30) [Compare Source](typescript-eslint/typescript-eslint@v8.57.2...v8.58.0) ##### 🚀 Features - support TypeScript 6 ([#​12124](typescript-eslint/typescript-eslint#12124)) ##### ❤️ Thank You - Evyatar Daud [@​StyleShit](https://github.com/StyleShit) See [GitHub Releases](https://github.com/typescript-eslint/typescript-eslint/releases/tag/v8.58.0) for more information. You can read about our [versioning strategy](https://typescript-eslint.io/users/versioning) and [releases](https://typescript-eslint.io/users/releases) on our website. </details> <details> <summary>eslint/eslint (eslint)</summary> ### [`v10.2.0`](https://github.com/eslint/eslint/releases/tag/v10.2.0) [Compare Source](eslint/eslint@v10.1.0...v10.2.0) #### Features - [`586ec2f`](eslint/eslint@586ec2f) feat: Add `meta.languages` support to rules ([#​20571](eslint/eslint#20571)) (Copilot) - [`14207de`](eslint/eslint@14207de) feat: add `Temporal` to `no-obj-calls` ([#​20675](eslint/eslint#20675)) (Pixel998) - [`bbb2c93`](eslint/eslint@bbb2c93) feat: add Temporal to ES2026 globals ([#​20672](eslint/eslint#20672)) (Pixel998) #### Bug Fixes - [`542cb3e`](eslint/eslint@542cb3e) fix: update first-party dependencies ([#​20714](eslint/eslint#20714)) (Francesco Trotta) #### Documentation - [`a2af743`](eslint/eslint@a2af743) docs: add `language` to configuration objects ([#​20712](eslint/eslint#20712)) (Francesco Trotta) - [`845f23f`](eslint/eslint@845f23f) docs: Update README (GitHub Actions Bot) - [`5fbcf59`](eslint/eslint@5fbcf59) docs: remove `sourceType` from ts playground link ([#​20477](eslint/eslint#20477)) (Tanuj Kanti) - [`8702a47`](eslint/eslint@8702a47) docs: Update README (GitHub Actions Bot) - [`ddeaded`](eslint/eslint@ddeaded) docs: Update README (GitHub Actions Bot) - [`2b44966`](eslint/eslint@2b44966) docs: add Major Releases section to Manage Releases ([#​20269](eslint/eslint#20269)) (Milos Djermanovic) - [`eab65c7`](eslint/eslint@eab65c7) docs: update `eslint` versions in examples ([#​20664](eslint/eslint#20664)) (루밀LuMir) - [`3e4a299`](eslint/eslint@3e4a299) docs: update ESM Dependencies policies with note for own-usage packages ([#​20660](eslint/eslint#20660)) (Milos Djermanovic) #### Chores - [`8120e30`](eslint/eslint@8120e30) refactor: extract no unmodified loop condition ([#​20679](eslint/eslint#20679)) (kuldeep kumar) - [`46e8469`](eslint/eslint@46e8469) chore: update dependency markdownlint-cli2 to ^0.22.0 ([#​20697](eslint/eslint#20697)) (renovate\[bot]) - [`01ed3aa`](eslint/eslint@01ed3aa) test: add unit tests for unicode utilities ([#​20622](eslint/eslint#20622)) (Manish chaudhary) - [`811f493`](eslint/eslint@811f493) ci: remove `--legacy-peer-deps` from types integration tests ([#​20667](eslint/eslint#20667)) (Milos Djermanovic) - [`6b86fcf`](eslint/eslint@6b86fcf) chore: update dependency npm-run-all2 to v8 ([#​20663](eslint/eslint#20663)) (renovate\[bot]) - [`632c4f8`](eslint/eslint@632c4f8) chore: add `prettier` update commit to `.git-blame-ignore-revs` ([#​20662](eslint/eslint#20662)) (루밀LuMir) - [`b0b0f21`](eslint/eslint@b0b0f21) chore: update dependency eslint-plugin-regexp to ^3.1.0 ([#​20659](eslint/eslint#20659)) (Milos Djermanovic) - [`228a2dd`](eslint/eslint@228a2dd) chore: update dependency eslint-plugin-eslint-plugin to ^7.3.2 ([#​20661](eslint/eslint#20661)) (Milos Djermanovic) - [`3ab4d7e`](eslint/eslint@3ab4d7e) test: Add tests for eslintrc-style keys ([#​20645](eslint/eslint#20645)) (kuldeep kumar) </details> <details> <summary>kulshekhar/ts-jest (ts-jest)</summary> ### [`v29.4.9`](https://github.com/kulshekhar/ts-jest/releases/tag/v29.4.9) [Compare Source](kulshekhar/ts-jest@v29.4.8...v29.4.9) Please refer to [CHANGELOG.md](https://github.com/kulshekhar/ts-jest/blob/main/CHANGELOG.md) for details. ### [`v29.4.8`](kulshekhar/ts-jest@v29.4.7...v29.4.8) [Compare Source](kulshekhar/ts-jest@v29.4.7...v29.4.8) ### [`v29.4.7`](https://github.com/kulshekhar/ts-jest/blob/HEAD/CHANGELOG.md#2947-2026-04-01) [Compare Source](kulshekhar/ts-jest@v29.4.6...v29.4.7) ##### Features - support TypeScript v6 ([eda517d](kulshekhar/ts-jest@eda517d)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 6am on monday" in timezone Europe/Amsterdam, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My44Ni4wIiwidXBkYXRlZEluVmVyIjoiNDMuODYuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsidHlwZS9taW5vciIsInR5cGUvcGF0Y2giXX0=--> Co-authored-by: Bernd Schorgers <me@bjw-s.dev> Reviewed-on: https://git.bjw-s.dev/bjw-s/action-changed-files/pulls/8 Co-authored-by: renovate[bot] <renovate-bot@noreply.git.bjw-s.dev> Co-committed-by: renovate[bot] <renovate-bot@noreply.git.bjw-s.dev>
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
When multiple entry points import the same export through a namespace re-export chain (
export * from ...), thenamespaceReexportsByNamecache causes an early return on subsequent lookups. This skipped the side-effect dependency recording for the new importer, resulting in dropped side effects (e.g. CSS imports, console statements) in the bundled output.Root cause: On a cache hit, the code returned the cached variable immediately without registering side-effect dependencies for the current
importerForSideEffects.Fix: A parallel
namespaceSideEffectsByNamecache is introduced alongsidenamespaceReexportsByName. On first resolution, the collected side-effect modules and cyclic reexporters are stored. On subsequent cache hits, the stored metadata is replayed for each new importer directly — avoiding redundant graph re-traversal while preserving correct side-effect tracking.Two new optional parameters (
sideEffectCollectorandcyclicReexporterCollector) are threaded throughgetVariableForExportNameandgetVariableFromNamespaceReexportsto gather this metadata during the initial traversal.The test covers two entry points that both import the same export through a namespace re-export chain including a side-effectful module, verifying that side effects are preserved for all importers across all four output formats (es, cjs, amd, system).