Skip side-effect-free external imports when hoisting is disabled#6411
Conversation
|
@morgan-coded is attempting to deploy a commit to the rollup-js Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a regression test and a rendering-time safeguard to prevent emitting spurious “side-effect imports” for side-effect-free external dependencies when pure chunks are merged with transitive import hoisting disabled (issue #6111).
Changes:
- Introduces a new chunking-form sample fixture plus expected outputs across formats.
- Exposes
moduleSideEffectsonExternalChunkfor easier decision-making at render time. - Skips rendering/removes external dependencies that are side-effect-free and have no imports/reexports when
hoistTransitiveImportsis disabled.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/chunking-form/samples/no-side-effect-free-external-imports/main.js | New entry that re-exports from an external module to exercise the scenario. |
| test/chunking-form/samples/no-side-effect-free-external-imports/hooks.js | Second entry re-exporting from two externals to force chunk merging conditions. |
| test/chunking-form/samples/no-side-effect-free-external-imports/_expected/system/main.js | Expected SystemJS output ensuring no spurious side-effect-only import is emitted. |
| test/chunking-form/samples/no-side-effect-free-external-imports/_expected/system/hooks.js | Expected SystemJS output for the hooks entry. |
| test/chunking-form/samples/no-side-effect-free-external-imports/_expected/es/main.js | Expected ESM output (should remain re-export-only). |
| test/chunking-form/samples/no-side-effect-free-external-imports/_expected/es/hooks.js | Expected ESM output for both externals. |
| test/chunking-form/samples/no-side-effect-free-external-imports/_expected/cjs/main.js | Expected CJS output to avoid unnecessary requires/imports beyond re-export getters. |
| test/chunking-form/samples/no-side-effect-free-external-imports/_expected/cjs/hooks.js | Expected CJS output for both externals. |
| test/chunking-form/samples/no-side-effect-free-external-imports/_expected/amd/main.js | Expected AMD output verifying only re-export wiring. |
| test/chunking-form/samples/no-side-effect-free-external-imports/_expected/amd/hooks.js | Expected AMD output for both externals. |
| test/chunking-form/samples/no-side-effect-free-external-imports/_config.js | Test configuration reproducing the bug conditions (merged pure chunks + hoisting disabled). |
| src/ExternalChunk.ts | Adds moduleSideEffects accessor used by chunk rendering logic. |
| src/Chunk.ts | Removes non-imported, non-reexported, side-effect-free external deps when hoisting is disabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…dencies The previous approach deleted side-effect-free external dependencies from the long-lived this.dependencies set as a side effect of getRenderedDependencies(). This created a phase-dependent invariant where readers of this.dependencies saw different contents before and after rendering, and could race across concurrent chunk renders (the renderDynamicImport plugin hook reads targetChunk.dependencies during another chunk's renderModules). Instead, leave this.dependencies untouched and derive chunk.imports from getRenderedDependencies().keys(), which already excludes the skipped dependencies. The rendered output and chunk.imports stay in sync with emitted code without mutating the dependency graph.
0bb9c7c to
64f763d
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
lukastaegert
left a comment
There was a problem hiding this comment.
Thanks for the PR! I generally agree with your assessment and think the general idea of the fix is fine. What I did not like is the implicit side effect of mutating dependencies in this.getRenderedDependencies(). I took the liberty to slightly change this so that dependencies are not mutated and instead relevant places check this.getRenderedDependencies() instead.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6411 +/- ##
=======================================
Coverage 98.78% 98.78%
=======================================
Files 275 275
Lines 10815 10820 +5
Branches 2884 2886 +2
=======================================
+ Hits 10684 10689 +5
Misses 89 89
Partials 42 42 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This PR has been released as part of rollup@4.62.2. 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/) | |---|---|---|---| | [@rollup/rollup-linux-x64-gnu](https://rollupjs.org/) ([source](https://github.com/rollup/rollup)) | [`4.62.0` → `4.62.2`](https://renovatebot.com/diffs/npm/@rollup%2frollup-linux-x64-gnu/4.62.0/4.62.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.61.1` → `8.62.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/8.61.1/8.62.0) |  |  | | [@typescript-eslint/parser](https://typescript-eslint.io/packages/parser) ([source](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser)) | [`8.61.1` → `8.62.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/8.61.1/8.62.0) |  |  | | [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | [`10.5.0` → `10.6.0`](https://renovatebot.com/diffs/npm/eslint/10.5.0/10.6.0) |  |  | | [eslint-plugin-jest](https://github.com/jest-community/eslint-plugin-jest) | [`29.15.2` → `29.15.3`](https://renovatebot.com/diffs/npm/eslint-plugin-jest/29.15.2/29.15.3) |  |  | | [globals](https://github.com/sindresorhus/globals) | [`17.6.0` → `17.7.0`](https://renovatebot.com/diffs/npm/globals/17.6.0/17.7.0) |  |  | | [prettier](https://prettier.io) ([source](https://github.com/prettier/prettier)) | [`3.8.4` → `3.9.1`](https://renovatebot.com/diffs/npm/prettier/3.8.4/3.9.1) |  |  | | [rollup](https://rollupjs.org/) ([source](https://github.com/rollup/rollup)) | [`4.62.0` → `4.62.2`](https://renovatebot.com/diffs/npm/rollup/4.62.0/4.62.2) |  |  | --- ### Release Notes <details> <summary>rollup/rollup (@​rollup/rollup-linux-x64-gnu)</summary> ### [`v4.62.2`](https://github.com/rollup/rollup/blob/HEAD/CHANGELOG.md#4622) [Compare Source](rollup/rollup@v4.62.1...v4.62.2) *2026-06-19* ##### Bug Fixes - Do not add spurious side-effect-free external imports to chunks when using minChunkSize ([#​6411](rollup/rollup#6411)) ##### Pull Requests - [#​6411](rollup/rollup#6411): Skip side-effect-free external imports when hoisting is disabled ([@​morgan-coded](https://github.com/morgan-coded), [@​lukastaegert](https://github.com/lukastaegert)) - [#​6416](rollup/rollup#6416): refactor(rust/parser\_ast): extract property AstConverter write buffer kind logic to new method ([@​fabianbernhart](https://github.com/fabianbernhart), [@​lukastaegert](https://github.com/lukastaegert)) ### [`v4.62.1`](https://github.com/rollup/rollup/blob/HEAD/CHANGELOG.md#4621) [Compare Source](rollup/rollup@v4.62.0...v4.62.1) *2026-06-19* ##### Bug Fixes - Preserve multipart file extensions when deconflicting output chunks ([#​6408](rollup/rollup#6408)) - Fix an issue where getLogFilter would match additional logs ([#​6415](rollup/rollup#6415)) ##### Pull Requests - [#​6393](rollup/rollup#6393): Use import attributes for importing JSON ([@​selfisekai](https://github.com/selfisekai), [@​lukastaegert](https://github.com/lukastaegert)) - [#​6408](rollup/rollup#6408): fix: insert conflict numbers before first extension in multi-extension filenames ([@​LeSingh1](https://github.com/LeSingh1), [@​lukastaegert](https://github.com/lukastaegert)) - [#​6415](rollup/rollup#6415): fix: advance value past wildcard prefix before suffix check in getLogFilter ([@​JSap0914](https://github.com/JSap0914), [@​lukastaegert](https://github.com/lukastaegert)) - [#​6417](rollup/rollup#6417): chore(deps): update msys2/setup-msys2 digest to [`66cd2cc`](rollup/rollup@66cd2cc) ([@​renovate](https://github.com/renovate)\[bot]) - [#​6418](rollup/rollup#6418): fix(deps): update minor/patch updates ([@​renovate](https://github.com/renovate)\[bot], [@​lukastaegert](https://github.com/lukastaegert)) - [#​6419](rollup/rollup#6419): chore(deps): update dependency eslint-plugin-unicorn to v66 ([@​renovate](https://github.com/renovate)\[bot]) - [#​6420](rollup/rollup#6420): chore(deps): lock file maintenance minor/patch updates ([@​renovate](https://github.com/renovate)\[bot], [@​lukastaegert](https://github.com/lukastaegert)) </details> <details> <summary>typescript-eslint/typescript-eslint (@​typescript-eslint/eslint-plugin)</summary> ### [`v8.62.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8620-2026-06-22) [Compare Source](typescript-eslint/typescript-eslint@v8.61.1...v8.62.0) ##### 🚀 Features - remove redundant package.json "files" ([#​12444](typescript-eslint/typescript-eslint#12444)) ##### ❤️ Thank You - Kirk Waiblinger [@​kirkwaiblinger](https://github.com/kirkwaiblinger) See [GitHub Releases](https://github.com/typescript-eslint/typescript-eslint/releases/tag/v8.62.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.62.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#8620-2026-06-22) [Compare Source](typescript-eslint/typescript-eslint@v8.61.1...v8.62.0) ##### 🚀 Features - remove redundant package.json "files" ([#​12444](typescript-eslint/typescript-eslint#12444)) ##### ❤️ Thank You - Kirk Waiblinger [@​kirkwaiblinger](https://github.com/kirkwaiblinger) See [GitHub Releases](https://github.com/typescript-eslint/typescript-eslint/releases/tag/v8.62.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.6.0`](https://github.com/eslint/eslint/releases/tag/v10.6.0) [Compare Source](eslint/eslint@v10.5.0...v10.6.0) #### Features - [`b1f9106`](eslint/eslint@b1f9106) feat: detect Symbol() and BigInt() in no-constant-binary-expression ([#​20981](eslint/eslint#20981)) (Taejin Kim) - [`f291007`](eslint/eslint@f291007) feat: add checkRelationalComparisons to no-constant-binary-expression ([#​20948](eslint/eslint#20948)) (sethamus) #### Bug Fixes - [`6b05784`](eslint/eslint@6b05784) fix: prefer-exponentiation-operator invalid autofix at statement start ([#​20997](eslint/eslint#20997)) (Milos Djermanovic) - [`bb9eb2a`](eslint/eslint@bb9eb2a) fix: account for shadowed `Boolean` in `no-extra-boolean-cast` ([#​21013](eslint/eslint#21013)) (den$) - [`8fd8741`](eslint/eslint@8fd8741) fix: don't report shadowed undefined in `radix` rule ([#​21011](eslint/eslint#21011)) (Pixel) - [`5784980`](eslint/eslint@5784980) fix: don't report shadowed undefined in no-throw-literal ([#​21010](eslint/eslint#21010)) (Pixel) - [`9cd1e6d`](eslint/eslint@9cd1e6d) fix: suppress invalid class suggestion in no-promise-executor-return ([#​21008](eslint/eslint#21008)) (Pixel) - [`d4eb2dc`](eslint/eslint@d4eb2dc) fix: don't report shadowed undefined in prefer-promise-reject-errors ([#​21006](eslint/eslint#21006)) (Pixel) - [`2360464`](eslint/eslint@2360464) fix: prefer-promise-reject-errors false positives for shadowed Promise ([#​21003](eslint/eslint#21003)) (den$) - [`63d52d2`](eslint/eslint@63d52d2) fix: restore max-classes-per-file report range ([#​21002](eslint/eslint#21002)) (Pixel) - [`7feaff0`](eslint/eslint@7feaff0) fix: callback detection logic for IIFEs in max-nested-callbacks ([#​20979](eslint/eslint#20979)) (fnx) - [`399a2ec`](eslint/eslint@399a2ec) fix: don't report inner non-callbacks in `max-nested-callbacks` ([#​20995](eslint/eslint#20995)) (Milos Djermanovic) #### Documentation - [`a83683d`](eslint/eslint@a83683d) docs: Update README (GitHub Actions Bot) - [`f5449f9`](eslint/eslint@f5449f9) docs: document userland patterns for global assertionOptions in RuleT… ([#​20986](eslint/eslint#20986)) (playgirl) - [`bea49f7`](eslint/eslint@bea49f7) docs: Update README (GitHub Actions Bot) - [`e5f70f9`](eslint/eslint@e5f70f9) docs: update code-path diagrams ([#​20984](eslint/eslint#20984)) (Tanuj Kanti) - [`8890c2d`](eslint/eslint@8890c2d) docs: add TypeScript config guidance for MCP server ([#​20796](eslint/eslint#20796)) (Pierluigi Lenoci) - [`3eb3d9b`](eslint/eslint@3eb3d9b) docs: Update README (GitHub Actions Bot) - [`c5bb59c`](eslint/eslint@c5bb59c) docs: Update README (GitHub Actions Bot) - [`eb3c97c`](eslint/eslint@eb3c97c) docs: fix grammar in prefer-const rule description ([#​20983](eslint/eslint#20983)) (lumir) #### Chores - [`6a42034`](eslint/eslint@6a42034) ci: run ecosystem tests on main branch ([#​20891](eslint/eslint#20891)) (sethamus) - [`3dbacdb`](eslint/eslint@3dbacdb) ci: bump actions/checkout from 6 to 7 ([#​21014](eslint/eslint#21014)) (dependabot\[bot]) - [`c3abfca`](eslint/eslint@c3abfca) chore: correct JSDoc param types in html formatter ([#​21018](eslint/eslint#21018)) (Minseon Kim) - [`a832320`](eslint/eslint@a832320) ci: split ecosystem tests into separate jobs ([#​21001](eslint/eslint#21001)) (xbinaryx) - [`27166e7`](eslint/eslint@27166e7) chore: update ecosystem plugins ([#​21005](eslint/eslint#21005)) (ESLint Bot) - [`865d76e`](eslint/eslint@865d76e) ci: bump pnpm/action-setup from 6.0.8 to 6.0.9 ([#​20989](eslint/eslint#20989)) (dependabot\[bot]) - [`27a88c9`](eslint/eslint@27a88c9) chore: update dependency markdown-it to v14 in root ([#​20994](eslint/eslint#20994)) (Milos Djermanovic) - [`970cea6`](eslint/eslint@970cea6) chore: update dependency markdown-it to v14 ([#​20993](eslint/eslint#20993)) (Milos Djermanovic) - [`b482120`](eslint/eslint@b482120) chore: update dependency prettier to v3.8.4 ([#​20990](eslint/eslint#20990)) (renovate\[bot]) - [`6993fb3`](eslint/eslint@6993fb3) chore: update ecosystem plugins ([#​20985](eslint/eslint#20985)) (ESLint Bot) </details> <details> <summary>jest-community/eslint-plugin-jest (eslint-plugin-jest)</summary> ### [`v29.15.3`](https://github.com/jest-community/eslint-plugin-jest/blob/HEAD/CHANGELOG.md#29153-2026-06-26) [Compare Source](jest-community/eslint-plugin-jest@v29.15.2...v29.15.3) ##### Bug Fixes - **no-export:** treat describe blocks as test files ([#​1978](jest-community/eslint-plugin-jest#1978)) ([70568b0](jest-community/eslint-plugin-jest@70568b0)) </details> <details> <summary>sindresorhus/globals (globals)</summary> ### [`v17.7.0`](https://github.com/sindresorhus/globals/releases/tag/v17.7.0) [Compare Source](sindresorhus/globals@v17.6.0...v17.7.0) - Update globals (2026-06-22) ([#​345](sindresorhus/globals#345)) [`33b75f9`](sindresorhus/globals@33b75f9) *** </details> <details> <summary>prettier/prettier (prettier)</summary> ### [`v3.9.1`](https://github.com/prettier/prettier/blob/HEAD/CHANGELOG.md#391) [Compare Source](prettier/prettier@3.9.0...3.9.1) [diff](prettier/prettier@3.9.0...3.9.1) ##### CLI: Fix ignored file has been cached incorrectly ([#​19483](prettier/prettier#19483) by [@​kovsu](https://github.com/kovsu)) Bug details [#​18016](prettier/prettier#18016) ### [`v3.9.0`](https://github.com/prettier/prettier/blob/HEAD/CHANGELOG.md#390) [Compare Source](prettier/prettier@3.8.5...3.9.0) [diff](prettier/prettier@3.8.5...3.9.0) 🔗 [Release Notes](https://prettier.io/blog/2026/06/27/3.9.0) ### [`v3.8.5`](https://github.com/prettier/prettier/blob/HEAD/CHANGELOG.md#385) [Compare Source](prettier/prettier@3.8.4...3.8.5) [diff](prettier/prettier@3.8.4...3.8.5) ##### Flow: Support `readonly` as a variance annotation ([#​19022](prettier/prettier#19022) by [@​marcoww6](https://github.com/marcoww6)) Flow now accepts `readonly` as a property variance annotation, equivalent to `+` (covariant/read-only). <!-- prettier-ignore --> ```jsx // Input type T = { readonly foo: string, }; // Prettier 3.8.4 SyntaxError // Prettier 3.8.5 type T = { readonly foo: string, }; ``` </details> --- ### Configuration 📅 **Schedule**: (in timezone Europe/Amsterdam) - Branch creation - "before 6am on monday" - 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](undefined) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4yMjIuMSIsInVwZGF0ZWRJblZlciI6IjQzLjIzNC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJ0eXBlL21pbm9yIiwidHlwZS9wYXRjaCJdfQ==--> Co-authored-by: Bernd Schorgers <me@bjw-s.dev> Co-authored-by: lab-assistant <lab-assistant@git.bjw-s.dev> Reviewed-on: https://git.bjw-s.dev/bjw-s/action-changed-files/pulls/56
When
hoistTransitiveImports: falseis used withexperimentalMinChunkSize, an entry chunk can pick up bare external imports from merged pure re-export chunks even though the entry never references those packages.This keeps binding-less, side-effect-free external chunks out of the rendered imports when hoisting is disabled, and keeps
chunk.importsin sync with the emitted code. The default hoisting path is left unchanged.I added chunking-form coverage for ES, CJS, AMD, and System output. The focused sample goes red on the issue shape and green with the fix; I also ran the full suite, eslint,
tsc --noEmit, andgit diff --check.Closes #6111