Skip to content

Conversation

@jerrykingxyz
Copy link
Contributor

@jerrykingxyz jerrykingxyz commented Oct 21, 2025

Summary

  1. Remove the Option in SideEffectsOptimizeArtifact value.
-  pub type SideEffectsOptimizeArtifact = UkeyMap<DependencyId, Option<SideEffectsDoOptimize>>
+  pub type SideEffectsOptimizeArtifact = UkeyMap<DependencyId, SideEffectsDoOptimize>;
  1. SideEffectsOptimizeArtifact retain the item which target module and deps exist when incremental rebuild.
side_effects_optimize_artifact.retain(|dependency_id, do_optimize| {
      let dep_exist = module_graph
        .connection_by_dependency_id(dependency_id)
        .is_some();
      let target_module_exist = module_graph
        .module_by_identifier(&do_optimize.target_module)
        .is_some();
      dep_exist && target_module_exist
    });

Related links

Checklist

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

Copilot AI review requested due to automatic review settings October 21, 2025 06:40
@netlify
Copy link

netlify bot commented Oct 21, 2025

Deploy Preview for rspack canceled.

Name Link
🔨 Latest commit 6be67c5
🔍 Latest deploy log https://app.netlify.com/projects/rspack/deploys/68fb5679f811b5000832697e

@github-actions github-actions bot added release: bug fix release: bug related release(mr only) team The issue/pr is created by the member of Rspack. labels Oct 21, 2025
Copy link
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 removing a library file during incremental compilation with side effects optimization enabled. The fix ensures proper handling of missing modules and dependencies in the optimization artifact, preventing the panic while maintaining correct incremental build behavior.

Key changes:

  • Modified side effects optimization to validate both dependency and target module existence before processing
  • Changed the optimization artifact type to eliminate Option wrapper, simplifying the data structure
  • Added comprehensive e2e test case demonstrating the fix for file removal scenarios

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/e2e/playwright.config.ts Added conditional check to prevent overriding incremental configuration when already defined
tests/e2e/cases/incremental/remove-optimized-module/rspack.config.js Test configuration enabling incremental caching with side effects optimization
tests/e2e/cases/incremental/remove-optimized-module/package.json Package configuration marking modules as side effect-free
tests/e2e/cases/incremental/remove-optimized-module/index.test.ts E2e test verifying no panic occurs when removing optimized modules
tests/e2e/cases/incremental/remove-optimized-module/index.js Test entry point importing component to be removed
tests/e2e/cases/incremental/remove-optimized-module/comp/index.js Component barrel export file
tests/e2e/cases/incremental/remove-optimized-module/comp/Button.js Component module that gets removed during test
crates/rspack_plugin_javascript/src/plugin/side_effects_flag_plugin.rs Core fix: validates module existence before processing and refactored artifact collection to use streaming approach
crates/rspack_core/src/module_graph/mod.rs Reformatted conditional to improve readability
crates/rspack_core/src/exports/target.rs Changed function visibility from public to private
crates/rspack_core/src/artifacts/side_effects_do_optimize_artifact.rs Simplified artifact type by removing Option wrapper

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 21, 2025

📦 Binary Size-limit

Comparing 6be67c5 to test: migrate cli tests to rstest (#11989) by 9aoy

🎉 Size decreased by 1.00KB from 47.85MB to 47.85MB (⬇️0.00%)

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 21, 2025

CodSpeed Performance Report

Merging #11939 will not alter performance

Comparing jerry/sideEffect (6be67c5) with main (941e220)

Summary

✅ 17 untouched

@jerrykingxyz jerrykingxyz force-pushed the jerry/sideEffect branch 2 times, most recently from 8441dd5 to dc1bedf Compare October 24, 2025 09:33
@ahabhgk ahabhgk merged commit ed6f4de into main Oct 27, 2025
95 of 103 checks passed
@ahabhgk ahabhgk deleted the jerry/sideEffect branch October 27, 2025 06:09
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