Skip side-effect-free external imports when hoisting is disabled#6411
Open
morgan-coded wants to merge 2 commits into
Open
Skip side-effect-free external imports when hoisting is disabled#6411morgan-coded wants to merge 2 commits into
morgan-coded wants to merge 2 commits into
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. |
Contributor
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.
Comment on lines
+1252
to
+1258
| if ( | ||
| imports === null && | ||
| reexports === null && | ||
| dependency instanceof ExternalChunk && | ||
| !dependency.moduleSideEffects && | ||
| !this.outputOptions.hoistTransitiveImports | ||
| ) { |
Comment on lines
1249
to
+1266
| for (const dependency of this.dependencies) { | ||
| const imports = importSpecifiers.get(dependency) || null; | ||
| const reexports = reexportSpecifiers.get(dependency) || null; | ||
| if ( | ||
| imports === null && | ||
| reexports === null && | ||
| dependency instanceof ExternalChunk && | ||
| !dependency.moduleSideEffects && | ||
| !this.outputOptions.hoistTransitiveImports | ||
| ) { | ||
| // A side-effect-free external dependency without imported or | ||
| // re-exported bindings can only be present because chunk assignment | ||
| // placed otherwise unrelated modules into this chunk. When transitive | ||
| // import hoisting is disabled, rendering it would emit a spurious | ||
| // side effect import, see https://github.com/rollup/rollup/issues/6111 | ||
| this.dependencies.delete(dependency); | ||
| continue; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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