refactor: support aliased and member runtime import shake#2437
Merged
Conversation
🦋 Changeset detectedLatest commit: 5dd3785 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
✅ Files skipped from review due to trivial changes (9)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughRecords runtime import bindings per-local-identifier and expands the shake transform's call-matching to handle aliased named imports and member calls from default/namespace runtime imports; adds/updates SWC snapshot and webpack tests accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
HuJean
changed the title
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/crates/swc_plugin_shake/lib.rs`:
- Around line 774-783: The test should_keep_use_effect_call_from_custom_runtime
contains a typo in the import specifier ("@lynx-js/custon-react-runtime");
update the import line inside that test's source string to
"@lynx-js/custom-react-runtime" so the package name matches the test name and
intent, i.e., fix the import specifier in the test string where useEffect is
imported.
🪄 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: 98def1e1-da8a-4ccd-b53d-9b195c5368a5
📒 Files selected for processing (7)
.changeset/fair-crews-lie.mdpackages/react/transform/crates/swc_plugin_shake/lib.rspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_keep_use_effect_call_from_custom_runtime.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_aliased_use_effect_call.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_aliased_use_effect_param.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_default_and_namespace_runtime_import_calls.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_default_and_namespace_runtime_import_params.js
Merging this PR will degrade performance by 26.94%
Performance Changes
Comparing 
Footnotes
|
React MTF Example#279 Bundle Size — 206.07KiB (+0.01%).5dd3785(current) vs 59d11b2 main#278(baseline) Bundle metrics
|
| Current #279 |
Baseline #278 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
 Cache Invalidation |
46.02% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
173 |
173 |
| Duplicate Modules |
67 |
67 |
| Duplicate Code |
45.76%(-0.02%) |
45.77% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 
1 regression
| Current #279 |
Baseline #278 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
94.84KiB (+0.03%) |
94.81KiB |
Bundle analysis report Branch p/refactor-shake Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#7146 Bundle Size — 236.82KiB (+0.01%).5dd3785(current) vs 59d11b2 main#7145(baseline) Bundle metrics
|
| Current #7146 |
Baseline #7145 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
38.44% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
179 |
179 |
| Duplicate Modules |
70 |
70 |
| Duplicate Code |
46.1%(-0.02%) |
46.11% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 regression
| Current #7146 |
Baseline #7145 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.06KiB (+0.03%) |
91.03KiB |
Bundle analysis report Branch p/refactor-shake Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#8721 Bundle Size — 730.24KiB (0%).5dd3785(current) vs 59d11b2 main#8720(baseline) Bundle metrics
|
| Current #8721 |
Baseline #8720 |
|
|---|---|---|
| Initial JS |
43.63KiB |
43.63KiB |
| Initial CSS |
2.16KiB |
2.16KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
8 |
8 |
| Assets |
10 |
10 |
| Modules |
148(-1.33%) |
150 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
34.69%(+0.03%) |
34.68% |
| Packages |
3 |
3 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #8721 |
Baseline #8720 |
|
|---|---|---|
| Other |
385.55KiB |
385.55KiB |
| JS |
342.53KiB |
342.53KiB |
| CSS |
2.16KiB |
2.16KiB |
Bundle analysis report Branch p/refactor-shake Project dashboard
Generated by RelativeCI Documentation Report issue
React External#265 Bundle Size — 590.13KiB (-0.08%).5dd3785(current) vs 59d11b2 main#264(baseline) Bundle metrics
|
| Current #265 |
Baseline #264 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
30.93% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
17 |
17 |
| Duplicate Modules |
5 |
5 |
| Duplicate Code |
8.59% |
8.59% |
| Packages |
0 |
0 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 
1 improvement
| Current #265 |
Baseline #264 |
|
|---|---|---|
| Other |
590.13KiB (-0.08%) |
590.57KiB |
Bundle analysis report Branch p/refactor-shake Project dashboard
Generated by RelativeCI Documentation Report issue
Collaborator
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03e44463d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/react/transform/crates/swc_plugin_shake/lib.rs (1)
774-776:⚠️ Potential issue | 🟡 MinorFix the package name typo in the custom-runtime keep-case.
@lynx-js/custon-react-runtimeis confusing here and does not match the test name.Proposed fix
- import { useEffect } from "@lynx-js/custon-react-runtime"; + import { useEffect } from "@lynx-js/custom-react-runtime";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_shake/lib.rs` around lines 774 - 776, The import string in the test named should_keep_use_effect_call_from_custom_runtime contains a typo: change the package name literal "@lynx-js/custon-react-runtime" to "@lynx-js/custom-react-runtime" so it matches the test name and intended custom runtime; update the import line within the test (the string in the r#"... import { useEffect } from ... "... block) to use the corrected package name.
🤖 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/crates/swc_plugin_shake/lib.rs`:
- Around line 774-776: The import string in the test named
should_keep_use_effect_call_from_custom_runtime contains a typo: change the
package name literal "@lynx-js/custon-react-runtime" to
"@lynx-js/custom-react-runtime" so it matches the test name and intended custom
runtime; update the import line within the test (the string in the r#"... import
{ useEffect } from ... "... block) to use the corrected package name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d819764-e973-4383-9582-6fc02b1b63d7
📒 Files selected for processing (12)
.changeset/fair-crews-lie.mdpackages/react/transform/crates/swc_plugin_shake/lib.rspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_keep_use_effect_call_from_custom_runtime.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_aliased_use_effect_call.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_aliased_use_effect_param.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_default_and_namespace_runtime_import_calls.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_default_and_namespace_runtime_import_params.jspackages/webpack/react-webpack-plugin/test/cases/compat/run-in-js/index.jspackages/webpack/react-webpack-plugin/test/cases/main-thread/use-effect/lynx-js-react.jspackages/webpack/react-webpack-plugin/test/cases/main-thread/use-effect/react.jspackages/webpack/react-webpack-plugin/test/cases/main-thread/use-imperative-handle/lynx-js-react.jspackages/webpack/react-webpack-plugin/test/cases/main-thread/use-imperative-handle/react.js
✅ Files skipped from review due to trivial changes (8)
- packages/webpack/react-webpack-plugin/test/cases/main-thread/use-imperative-handle/react.js
- packages/webpack/react-webpack-plugin/test/cases/compat/run-in-js/index.js
- packages/webpack/react-webpack-plugin/test/cases/main-thread/use-effect/react.js
- .changeset/fair-crews-lie.md
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_keep_use_effect_call_from_custom_runtime.js
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_remove_default_and_namespace_runtime_import_calls.js
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_remove_aliased_use_effect_call.js
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_remove_default_and_namespace_runtime_import_params.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_remove_aliased_use_effect_param.js
Contributor
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/crates/swc_plugin_shake/lib.rs`:
- Around line 191-212: The current check treats default and namespace imports
the same, allowing member-call removal for default imports (symbols: import_ids,
object_ident, imported_name, member.prop, target_calls), which is unsafe because
default-import objects can be reassigned; change the logic so the optimization
only runs for namespace imports (imported_name == "*") rather than both
"default" and "*", i.e., require imported_name == "*" before matching MemberProp
(MemberProp::Ident / MemberProp::Computed) and checking target_calls;
alternatively (if you prefer) implement reassignment tracking for default-import
properties, but for a minimal safe fix restrict to "*" only.
🪄 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: a2f99646-72a8-4b71-b344-faf8e16bb894
📒 Files selected for processing (12)
.changeset/fair-crews-lie.mdpackages/react/transform/crates/swc_plugin_shake/lib.rspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_keep_use_effect_call_from_custom_runtime.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_aliased_use_effect_call.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_aliased_use_effect_param.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_default_and_namespace_runtime_import_calls.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_default_and_namespace_runtime_import_params.jspackages/webpack/react-webpack-plugin/test/cases/compat/run-in-js/index.jspackages/webpack/react-webpack-plugin/test/cases/main-thread/use-effect/lynx-js-react.jspackages/webpack/react-webpack-plugin/test/cases/main-thread/use-effect/react.jspackages/webpack/react-webpack-plugin/test/cases/main-thread/use-imperative-handle/lynx-js-react.jspackages/webpack/react-webpack-plugin/test/cases/main-thread/use-imperative-handle/react.js
✅ Files skipped from review due to trivial changes (7)
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_remove_aliased_use_effect_param.js
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_keep_use_effect_call_from_custom_runtime.js
- packages/webpack/react-webpack-plugin/test/cases/main-thread/use-effect/lynx-js-react.js
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_remove_default_and_namespace_runtime_import_calls.js
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_remove_aliased_use_effect_call.js
- packages/webpack/react-webpack-plugin/test/cases/main-thread/use-imperative-handle/lynx-js-react.js
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_remove_default_and_namespace_runtime_import_params.js
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/webpack/react-webpack-plugin/test/cases/main-thread/use-imperative-handle/react.js
- .changeset/fair-crews-lie.md
- packages/webpack/react-webpack-plugin/test/cases/compat/run-in-js/index.js
- packages/webpack/react-webpack-plugin/test/cases/main-thread/use-effect/react.js
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/react/transform/crates/swc_plugin_shake/lib.rs (1)
197-200:⚠️ Potential issue | 🟠 MajorRestrict member-call shaking to namespace imports.
import Runtime from "..."; Runtime.useEffect = keep; Runtime.useEffect()is valid, but this branch treatsRuntimethe same as a namespace import and strips the second call. That makes the transform unsound for default imports. The new default-import tests at Lines 725-766 should flip to keep/regression cases once this is fixed.Minimal safe fix
- // Only `default import` and `namespace import` can legally produce runtime member calls. - if imported_name != "default" && imported_name != "*" { + // Namespace imports are read-only module namespace objects. + // Default imports may reference mutable objects, so member-call shaking is unsafe there. + if imported_name != "*" { return false; }In ECMAScript modules, can code mutate properties on an object received through `import foo from "mod"`? Are properties on `import * as ns from "mod"` effectively read-only/live bindings in the importing module?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_shake/lib.rs` around lines 197 - 200, The member-call shaking currently treats default imports like namespace imports by checking imported_name != "default" && imported_name != "*"; change the check so only namespace imports (imported_name == "*") are allowed to produce runtime member calls, i.e., remove/default-exclude "default" from the allowed set in the relevant branch in lib.rs (the variable imported_name and the guard in the runtime member-call pruning logic). Update the tests referenced (the new default-import tests at lines ~725-766) to flip to keep/regression expectations once this restriction is applied.
🤖 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/crates/swc_plugin_shake/lib.rs`:
- Around line 197-200: The member-call shaking currently treats default imports
like namespace imports by checking imported_name != "default" && imported_name
!= "*"; change the check so only namespace imports (imported_name == "*") are
allowed to produce runtime member calls, i.e., remove/default-exclude "default"
from the allowed set in the relevant branch in lib.rs (the variable
imported_name and the guard in the runtime member-call pruning logic). Update
the tests referenced (the new default-import tests at lines ~725-766) to flip to
keep/regression expectations once this restriction is applied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6bc982c4-df25-42b8-998a-d954485877e8
📒 Files selected for processing (14)
.changeset/fair-crews-lie.mdpackages/react/transform/crates/swc_plugin_shake/lib.rspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_keep_use_effect_call_from_custom_runtime.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_aliased_use_effect_call.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_aliased_use_effect_call_by_local_name.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_aliased_use_effect_param.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_aliased_use_effect_param_by_local_name.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_default_and_namespace_runtime_import_calls.jspackages/react/transform/crates/swc_plugin_shake/tests/__swc_snapshots__/lib.rs/should_remove_default_and_namespace_runtime_import_params.jspackages/webpack/react-webpack-plugin/test/cases/compat/run-in-js/index.jspackages/webpack/react-webpack-plugin/test/cases/main-thread/use-effect/lynx-js-react.jspackages/webpack/react-webpack-plugin/test/cases/main-thread/use-effect/react.jspackages/webpack/react-webpack-plugin/test/cases/main-thread/use-imperative-handle/lynx-js-react.jspackages/webpack/react-webpack-plugin/test/cases/main-thread/use-imperative-handle/react.js
✅ Files skipped from review due to trivial changes (7)
- .changeset/fair-crews-lie.md
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_remove_aliased_use_effect_param.js
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_remove_aliased_use_effect_call.js
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_remove_aliased_use_effect_call_by_local_name.js
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_remove_aliased_use_effect_param_by_local_name.js
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_keep_use_effect_call_from_custom_runtime.js
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_remove_default_and_namespace_runtime_import_calls.js
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/react/transform/crates/swc_plugin_shake/tests/swc_snapshots/lib.rs/should_remove_default_and_namespace_runtime_import_params.js
- packages/webpack/react-webpack-plugin/test/cases/compat/run-in-js/index.js
- packages/webpack/react-webpack-plugin/test/cases/main-thread/use-effect/lynx-js-react.js
- packages/webpack/react-webpack-plugin/test/cases/main-thread/use-effect/react.js
- packages/webpack/react-webpack-plugin/test/cases/main-thread/use-imperative-handle/react.js
- packages/webpack/react-webpack-plugin/test/cases/main-thread/use-imperative-handle/lynx-js-react.js
gaoachao
approved these changes
Apr 8, 2026
Yradex
approved these changes
Apr 9, 2026
upupming
approved these changes
Apr 9, 2026
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
4 participants
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.
Summary by CodeRabbit
Bug Fixes
Tests
Chores
Checklist