feat(react-transform): add removeCall support#2423
Conversation
🦋 Changeset detectedLatest commit: d6d1899 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Merging this PR will improve performance by 16.49%
Performance Changes
Comparing Footnotes
|
React External#249 Bundle Size — 590.57KiB (~-0.01%).d6d1899(current) vs 5f2cea3 main#230(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch feat-react-transform-remove-call Project dashboard Generated by RelativeCI Documentation Report issue |
React MTF Example#263 Bundle Size — 206.05KiB (0%).d6d1899(current) vs 5f2cea3 main#244(baseline) Bundle metrics
|
| Current #263 |
Baseline #244 |
|
|---|---|---|
0B |
0B |
|
0B |
0B |
|
0% |
0% |
|
0 |
0 |
|
3 |
3 |
|
173 |
173 |
|
67 |
67 |
|
45.77% |
45.77% |
|
2 |
2 |
|
0 |
0 |
Bundle size by type no changes
| Current #263 |
Baseline #244 |
|
|---|---|---|
111.23KiB |
111.23KiB |
|
94.81KiB |
94.81KiB |
Bundle analysis report Branch feat-react-transform-remove-call Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#8705 Bundle Size — 728.84KiB (0%).d6d1899(current) vs 5f2cea3 main#8686(baseline) Bundle metrics
Bundle size by type
|
| Current #8705 |
Baseline #8686 |
|
|---|---|---|
384.62KiB |
384.62KiB |
|
342.07KiB |
342.07KiB |
|
2.16KiB |
2.16KiB |
Bundle analysis report Branch feat-react-transform-remove-call Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#7130 Bundle Size — 236.79KiB (-0.02%).d6d1899(current) vs 5f2cea3 main#7111(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch feat-react-transform-remove-call Project dashboard Generated by RelativeCI Documentation Report issue |
5e4f3c5 to
2c7d396
Compare
📝 WalkthroughWalkthroughIntroduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2c7d396 to
e30fb69
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/transform/swc-plugin-reactlynx/index.d.ts`:
- Around line 84-109: The ShakeVisitorConfig interface currently requires
removeCall; change it to be optional so callers can rely on runtime
defaults—update the declaration for removeCall to be optional (e.g.,
removeCall?: Array<string>) and likewise make the other fields that are
normalized at runtime (pkgName, retainProp, removeCallParams) optional on the
interface so the type matches the runtime normalization in
packages/webpack/react-webpack-plugin/src/loaders/options.ts; keep the element
types the same, only change required -> optional.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 790c5793-b435-42fd-8451-e36c2d79a4d0
📒 Files selected for processing (12)
.changeset/feat-react-transform-remove-call.mdpackages/react/transform/crates/swc_plugin_shake/lib.rspackages/react/transform/crates/swc_plugin_shake/napi.rspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_not_remove_call_in_scope_id.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_use_effect_call.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_use_effect_param.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_replace_use_effect_call_with_undefined.jspackages/react/transform/index.d.tspackages/react/transform/swc-plugin-reactlynx/index.d.tspackages/rspeedy/plugin-react/etc/react-rsbuild-plugin.api.mdpackages/webpack/react-refresh-webpack-plugin/package.jsonpackages/webpack/react-webpack-plugin/src/loaders/options.ts
e30fb69 to
8f20056
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/react/transform/swc-plugin-reactlynx/index.d.ts (1)
84-109:⚠️ Potential issue | 🟡 MinorMake
removeCalloptional inShakeVisitorConfig.The
removeCallfield (and other fields likepkgName,retainProp,removeCallParams) should be optional since:
- The documentation states "The provided values will be merged with the default values"
- Runtime normalization handles missing values with defaults
- Required fields break backward compatibility for existing code constructing this interface
💡 Suggested fix
- removeCall: Array<string>; + removeCall?: Array<string>;Consider making other fields optional as well for consistency with the merge-with-defaults behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/swc-plugin-reactlynx/index.d.ts` around lines 84 - 109, The ShakeVisitorConfig interface currently declares removeCall as required; make removeCall optional (removeCall?: Array<string>) and likewise mark related fields that are merged with defaults—pkgName, retainProp, removeCallParams—as optional in the ShakeVisitorConfig type so callers can omit them and runtime normalization will supply defaults; update the declaration of ShakeVisitorConfig (and any other related config interfaces in this file) to use optional properties to preserve backward compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/react/transform/swc-plugin-reactlynx/index.d.ts`:
- Around line 84-109: The ShakeVisitorConfig interface currently declares
removeCall as required; make removeCall optional (removeCall?: Array<string>)
and likewise mark related fields that are merged with defaults—pkgName,
retainProp, removeCallParams—as optional in the ShakeVisitorConfig type so
callers can omit them and runtime normalization will supply defaults; update the
declaration of ShakeVisitorConfig (and any other related config interfaces in
this file) to use optional properties to preserve backward compatibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 648413a5-e891-464d-b76e-8d00a957ad94
📒 Files selected for processing (13)
.changeset/feat-react-transform-remove-call.mdpackages/react/transform/crates/swc_plugin_shake/lib.rspackages/react/transform/crates/swc_plugin_shake/napi.rspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_not_remove_call_in_scope_id.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_use_effect_call.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_use_effect_param.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_replace_use_effect_call_with_undefined.jspackages/react/transform/index.d.tspackages/react/transform/swc-plugin-reactlynx/index.d.tspackages/rspeedy/plugin-react/etc/react-rsbuild-plugin.api.mdpackages/rspeedy/plugin-react/package.jsonpackages/webpack/react-refresh-webpack-plugin/package.jsonpackages/webpack/react-webpack-plugin/src/loaders/options.ts
✅ Files skipped from review due to trivial changes (7)
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_remove_use_effect_param.js
- packages/rspeedy/plugin-react/package.json
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_replace_use_effect_call_with_undefined.js
- packages/webpack/react-refresh-webpack-plugin/package.json
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_not_remove_call_in_scope_id.js
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_remove_use_effect_call.js
- .changeset/feat-react-transform-remove-call.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/webpack/react-webpack-plugin/src/loaders/options.ts
- packages/rspeedy/plugin-react/etc/react-rsbuild-plugin.api.md
8f20056 to
d6d1899
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/transform/crates/swc_plugin_shake/lib.rs`:
- Around line 85-88: The docs claim removeCall values are merged with defaults
but the N-API path currently forwards ShakeVisitorConfig verbatim, so removeCall
replacement occurs; fix by merging the provided removeCall list with the default
hook list before constructing/passing ShakeVisitorConfig in the N-API code path
(the logic around where ShakeVisitorConfig is built/forwarded in napi.rs), or
alternatively update the docs to say removeCall replaces defaults; reference the
removeCall option and the ShakeVisitorConfig construction in napi.rs and
implement a merge (dedupe + prepend/append as desired) when building the config.
- Around line 171-173: The current should_remove_call only checks the local
callee name against target_calls, so aliased imports (local binding like `ue`
for `useEffect`) won't match; update the importer to record a mapping of local
binding id -> original imported name (e.g., import_map: HashMap<Id, String>) and
then change should_remove_call to: look up the callee's local Id
(fn_name.to_id()) in import_ids/import_map, and if present check
import_map.get(&id) (or the original name) against target_calls (fn_name.sym may
be the local alias, so use the mapped original name for comparison). Ensure the
mapping is populated where imports are collected and used in should_remove_call.
- Around line 199-203: The current transformation in visit_mut_expr replaces
removed calls with a plain identifier Expr::Ident(Ident::new("undefined"...)),
which can be shadowed by local bindings; change this to emit the expression void
0 instead. Locate visit_mut_expr (the branch matching Expr::Call and the call to
should_remove_call) and replace the Expr::Ident("undefined") construction with a
unary void expression (Unary op Void with numeric 0 as the argument) so the
replacement is immune to shadowing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c20a263-f516-4b44-9963-e290dc7ca860
📒 Files selected for processing (12)
.changeset/feat-react-transform-remove-call.mdpackages/react/transform/crates/swc_plugin_shake/lib.rspackages/react/transform/crates/swc_plugin_shake/napi.rspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_not_remove_call_in_scope_id.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_use_effect_call.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_use_effect_param.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_replace_use_effect_call_with_undefined.jspackages/react/transform/index.d.tspackages/react/transform/swc-plugin-reactlynx/index.d.tspackages/rspeedy/plugin-react/etc/react-rsbuild-plugin.api.mdpackages/webpack/react-refresh-webpack-plugin/package.jsonpackages/webpack/react-webpack-plugin/src/loaders/options.ts
✅ Files skipped from review due to trivial changes (6)
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_replace_use_effect_call_with_undefined.js
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_remove_use_effect_param.js
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_not_remove_call_in_scope_id.js
- packages/webpack/react-refresh-webpack-plugin/package.json
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_remove_use_effect_call.js
- .changeset/feat-react-transform-remove-call.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react/transform/swc-plugin-reactlynx/index.d.ts
- packages/react/transform/crates/swc_plugin_shake/napi.rs
Summary by CodeRabbit
New Features
removeCallconfiguration option that removes matched runtime hook calls and replaces them withundefinedin expression contexts.Documentation
Checklist