Skip to content

fix: should not panic if ref-cjs is removed#12147

Merged
JSerFeng merged 1 commit intomainfrom
fix/panic-in-treeshaking
Nov 11, 2025
Merged

fix: should not panic if ref-cjs is removed#12147
JSerFeng merged 1 commit intomainfrom
fix/panic-in-treeshaking

Conversation

@JSerFeng
Copy link
Copy Markdown
Contributor

Summary

If ref cjs is optimized, should not get its chunk directly as it does not exist in any chunks

Related links

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

Copilot AI review requested due to automatic review settings November 11, 2025 08:07
@netlify
Copy link
Copy Markdown

netlify bot commented Nov 11, 2025

Deploy Preview for rspack canceled.

Name Link
🔨 Latest commit 772410f
🔍 Latest deploy log https://app.netlify.com/projects/rspack/deploys/6912eeaf803c5f0008cb8ed2

@github-actions github-actions bot added team The issue/pr is created by the member of Rspack. release: bug fix release: bug related release(mr only) labels Nov 11, 2025
Copy link
Copy Markdown
Contributor

Copilot AI left a 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 fixes a panic that occurs when ref-cjs modules are optimized away during tree-shaking. The fix ensures that the code checks whether a connection is active before attempting to access the module identifier, preventing crashes when modules have been removed through optimization.

  • Added check to skip modules that aren't in any chunks during preserve_modules processing
  • Updated dependency handling to use connections and verify they're active before accessing module identifiers
  • Added test case for tree-shaking side-effect-free modules

Reviewed Changes

Copilot reviewed 3 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
crates/rspack_plugin_esm_library/src/link.rs Added checks for inactive connections and modules not in chunks to prevent panics when accessing optimized-away modules
tests/rspack-test/esmOutputCases/basic/tree-shaking/index.js Test file importing a side-effect-free module to validate tree-shaking behavior
tests/rspack-test/esmOutputCases/basic/tree-shaking/node_modules/side-effect-free/package.json Package configuration marking module as side-effect-free
tests/rspack-test/esmOutputCases/basic/tree-shaking/node_modules/side-effect-free/index.js Empty module file for tree-shaking test
tests/rspack-test/esmOutputCases/basic/tree-shaking/snapshots/esm.snap.txt Expected output snapshot showing tree-shaken result

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link
Copy Markdown
Contributor

📦 Binary Size-limit

Comparing 772410f to fix: mf container entry use startup to load initial chunks (#12142) by Gengkun

❌ Size increased by 384bytes from 48.09MB to 48.09MB (⬆️0.00%)

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Nov 11, 2025

CodSpeed Performance Report

Merging #12147 will not alter performance

Comparing fix/panic-in-treeshaking (772410f) with main (f70f870)

Summary

✅ 17 untouched

@JSerFeng JSerFeng merged commit 6db19fe into main Nov 11, 2025
53 checks passed
@JSerFeng JSerFeng deleted the fix/panic-in-treeshaking branch November 11, 2025 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release: bug fix release: bug related release(mr only) team The issue/pr is created by the member of Rspack.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants