-
Notifications
You must be signed in to change notification settings - Fork 109
refactor: extract swc_plugin_inject & swc_plugins_shared::css
#1782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: cb88d37 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
📝 WalkthroughWalkthroughAdds a new swc_plugin_inject crate with a napi submodule exposing N-API types and wrappers, updates workspace Cargo.toml entries and shared crate dependencies, exports a css module in swc_plugins_shared, adjusts import paths across the react/transform crate, and adds a minimal changeset file. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/react/transform/crates/swc_plugin_inject/napi.rs (2)
49-53: Fix copy‑paste in error message (“IsModuleConfig” → “InjectAs”).Wrong enum name in the fallback error confuses callers.
- "IsModuleConfig" + "InjectAs"
1-1: Remove unused Debug import.fmt::Debug is not used; derive(Debug) doesn’t require it.
-use std::{collections::HashMap, fmt::Debug}; +use std::collections::HashMap;packages/react/transform/crates/swc_plugins_shared/css.rs (1)
20-41: Prefer iter + filter_map over flat_map(Result) for clarity.This avoids relying on Result’s IntoIterator semantics and reduces cloning.
- serde_json::Value::Object(map) => Some( - map - .into_iter() - .flat_map(|(k, v)| match &v { - serde_json::Value::Number(v) => Ok(format!( - "{}:{}", - k.from_case(Case::Camel).to_case(Case::Kebab), - v - )), - serde_json::Value::String(v) => Ok(format!( - "{}:{}", - k.from_case(Case::Camel).to_case(Case::Kebab), - v - )), - - serde_json::Value::Null - | serde_json::Value::Bool(_) - | serde_json::Value::Array(_) - | serde_json::Value::Object(_) => Err(()), - }) - .collect::<Vec<_>>() - .join(";"), - ), + serde_json::Value::Object(map) => { + let decls = map + .iter() + .filter_map(|(k, v)| { + let prop = k.from_case(Case::Camel).to_case(Case::Kebab); + match v { + serde_json::Value::Number(n) => Some(format!("{}:{}", prop, n)), + serde_json::Value::String(s) => Some(format!("{}:{}", prop, s)), + _ => None, + } + }) + .collect::<Vec<_>>() + .join(";"); + Some(decls) + },packages/react/transform/crates/swc_plugin_inject/Cargo.toml (1)
1-13: Trim swc_core feature set to cut compile time and binary size.This crate doesn’t use CSS or React transforms. Recommend narrowing features.
Apply:
-[dependencies] -napi = { workspace = true } -napi-derive = { workspace = true } -swc_core = { workspace = true, features = ["base", "ecma_codegen", "ecma_parser", "ecma_minifier", "ecma_transforms_typescript", "ecma_utils", "ecma_quote", "ecma_transforms_react", "ecma_transforms_optimization", "css_parser", "css_ast", "css_visit", "css_codegen", "__visit", "__testing_transform"] } -swc_plugins_shared = { path = "../swc_plugins_shared" } +[dependencies] +napi = { workspace = true } +napi-derive = { workspace = true } +# Only what InjectVisitor uses: parser, quote, utils, resolver(=base), visit, codegen for tests +swc_core = { workspace = true, features = ["base", "ecma_codegen", "ecma_parser", "ecma_utils", "ecma_quote", "__visit"] } +swc_plugins_shared = { path = "../swc_plugins_shared" }Optionally prevent accidental publish:
[package] name = "swc_plugin_inject" version = "0.1.0" edition = "2021" +publish = falsepackages/react/transform/crates/swc_plugin_inject/lib.rs (1)
14-15: Guard against crate/module name shadowing fornapi.With a module named
napi, references to the externalnapicrate inside that module should use absolute paths (e.g.,::napi::...) or alias the crate to avoid ambiguity.Suggested top-of-file in napi.rs:
+// Inside crates/swc_plugin_inject/napi.rs +use ::napi as napi_crate; +use ::napi_derive::napi;Then reference
napi_crate::Either,napi_crate::Result, etc.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.changeset/silver-pandas-teach.md(1 hunks)packages/react/transform/Cargo.toml(1 hunks)packages/react/transform/crates/swc_plugin_inject/Cargo.toml(1 hunks)packages/react/transform/crates/swc_plugin_inject/lib.rs(2 hunks)packages/react/transform/crates/swc_plugin_inject/napi.rs(1 hunks)packages/react/transform/crates/swc_plugins_shared/Cargo.toml(1 hunks)packages/react/transform/crates/swc_plugins_shared/css.rs(1 hunks)packages/react/transform/crates/swc_plugins_shared/lib.rs(1 hunks)packages/react/transform/src/lib.rs(1 hunks)packages/react/transform/src/swc_plugin_compat_post/mod.rs(1 hunks)packages/react/transform/src/swc_plugin_snapshot/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
For contributions, always generate a changeset and commit the resulting markdown file(s)
Files:
.changeset/silver-pandas-teach.md
🧠 Learnings (5)
📚 Learning: 2025-09-10T11:42:36.855Z
Learnt from: gaoachao
PR: lynx-family/lynx-stack#1714
File: packages/react/transform/Cargo.toml:19-19
Timestamp: 2025-09-10T11:42:36.855Z
Learning: In packages/react/transform/Cargo.toml, the crate uses serde derive macros (#[derive(Serialize, Deserialize)]) in multiple files including src/esbuild.rs and src/swc_plugin_extract_str/mod.rs, so the "derive" feature is required when migrating to workspace dependencies.
Applied to files:
packages/react/transform/crates/swc_plugin_inject/Cargo.tomlpackages/react/transform/Cargo.tomlpackages/react/transform/crates/swc_plugins_shared/Cargo.tomlpackages/react/transform/src/lib.rs
📚 Learning: 2025-09-12T09:43:04.847Z
Learnt from: gaoachao
PR: lynx-family/lynx-stack#1736
File: .changeset/spotty-experts-smoke.md:1-3
Timestamp: 2025-09-12T09:43:04.847Z
Learning: In the lynx-family/lynx-stack repository, empty changeset files (containing only `---\n\n---`) are used for internal changes that modify src/** files but don't require meaningful release notes, such as private package changes or testing-only modifications. This satisfies CI requirements without generating user-facing release notes.
Applied to files:
.changeset/silver-pandas-teach.md
📚 Learning: 2025-07-22T09:23:07.797Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:23:07.797Z
Learning: In the lynx-family/lynx-stack repository, changesets are only required for meaningful changes to end-users such as bugfixes and features. Internal/development changes like chores, refactoring, or removing debug info do not need changeset entries.
Applied to files:
.changeset/silver-pandas-teach.md
📚 Learning: 2025-09-18T04:43:54.407Z
Learnt from: gaoachao
PR: lynx-family/lynx-stack#1771
File: packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/basic_component_with_static_sibling.js:2-2
Timestamp: 2025-09-18T04:43:54.407Z
Learning: In packages/react/transform/src/swc_plugin_compat/mod.rs, the `add_pure_comment` function at lines 478-482 is specifically for `wrapWithLynxComponent` calls, not `createSnapshot` calls. The PURE comment injection for `createSnapshot` is handled separately in swc_plugin_snapshot/mod.rs. These are two distinct code paths that should be treated differently.
Applied to files:
packages/react/transform/src/swc_plugin_snapshot/mod.rspackages/react/transform/src/swc_plugin_compat_post/mod.rs
📚 Learning: 2025-09-18T04:43:54.407Z
Learnt from: gaoachao
PR: lynx-family/lynx-stack#1771
File: packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/basic_component_with_static_sibling.js:2-2
Timestamp: 2025-09-18T04:43:54.407Z
Learning: In the lynx-family/lynx-stack repository, the `add_pure_comment` function in packages/react/transform/src/swc_plugin_compat/mod.rs (around lines 478-482) is specifically for `wrapWithLynxComponent` calls, not `createSnapshot` calls. The PURE comment injection for `createSnapshot` is handled separately in swc_plugin_snapshot/mod.rs.
Applied to files:
packages/react/transform/src/swc_plugin_snapshot/mod.rs
🧬 Code graph analysis (6)
packages/react/transform/crates/swc_plugins_shared/css.rs (1)
packages/react/transform/crates/swc_plugins_shared/utils.rs (1)
jsonify(6-61)
packages/react/transform/src/swc_plugin_snapshot/mod.rs (2)
packages/react/transform/crates/swc_plugins_shared/css.rs (1)
get_string_inline_style_from_literal(14-64)packages/react/transform/crates/swc_plugins_shared/utils.rs (1)
calc_hash(63-69)
packages/react/transform/src/swc_plugin_compat_post/mod.rs (1)
packages/react/transform/index.d.ts (2)
DarkModeConfig(47-53)InjectVisitorConfig(461-463)
packages/react/transform/crates/swc_plugin_inject/napi.rs (3)
packages/react/transform/src/lib.rs (10)
std(778-778)std(778-778)from_napi_value(85-108)from_napi_value(146-159)from_napi_value(190-220)from(129-131)from(184-186)default(135-142)default(271-293)new(313-315)packages/react/transform/index.d.ts (1)
InjectVisitorConfig(461-463)packages/react/transform/crates/swc_plugin_inject/lib.rs (4)
default(30-34)new(46-54)visit_mut_expr(58-135)visit_mut_module(137-160)
packages/react/transform/crates/swc_plugin_inject/lib.rs (1)
packages/react/transform/index.d.ts (1)
InjectVisitorConfig(461-463)
packages/react/transform/src/lib.rs (1)
packages/react/transform/index.d.ts (1)
InjectVisitorConfig(461-463)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / Build (Windows)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: test-rust / Test (Ubuntu)
🔇 Additional comments (10)
.changeset/silver-pandas-teach.md (1)
1-3: Empty changeset looks intentional; confirm policy alignment.Per retrieved learnings for this repo, empty changesets are sometimes used for internal refactors to satisfy CI; in other cases changesets are omitted for refactors. Since this PR restructures crates and public module paths, please confirm whether an entry with an explicit release type is required or if the empty stub is still preferred.
We used your stored learnings to inform this check. If this affects external consumers (e.g., import path changes), consider marking a changeset with an appropriate bump, otherwise keep this stub.
packages/react/transform/crates/swc_plugins_shared/Cargo.toml (1)
10-10: LGTM: add convert_case via workspace.Dependency addition matches new css helpers’ needs and stays within workspace constraints.
packages/react/transform/crates/swc_plugin_inject/napi.rs (1)
136-142: Confirm whether InjectVisitor constructor needs N‑API export.If JS creates the visitor directly, mark new with #[napi] and adjust signature; if only Rust creates it, current visibility is fine. Please confirm call path.
packages/react/transform/crates/swc_plugins_shared/lib.rs (1)
1-1: LGTM: expose css module publicly.Matches internal path updates in css.rs and enables swc_plugins_shared::css usage.
packages/react/transform/crates/swc_plugins_shared/css.rs (1)
10-10: LGTM: prefer crate‑relative import for jsonify.Corrects the internal path; avoids circular dependency risks.
packages/react/transform/src/swc_plugin_snapshot/mod.rs (1)
26-27: Import path relocation to swc_plugins_shared::css/utils is correct.No behavior change; keeping css utilities in shared crate improves module boundaries.
packages/react/transform/src/swc_plugin_compat_post/mod.rs (1)
11-12: Updated import path aligns with new napi surface.The move to swc_plugin_inject::napi::{...} is consistent and scoped.
packages/react/transform/src/lib.rs (1)
67-67: Import path update to swc_plugin_inject::napi is correct.No API change; downstream N-API surface remains stable.
packages/react/transform/crates/swc_plugin_inject/lib.rs (1)
189-190: Tests now import core types from crate root.Matches the split where N-API wrappers live under
napiwhile core stays at crate root.packages/react/transform/Cargo.toml (1)
24-24: Workspace membership verified — no action requiredRoot Cargo.toml includes members = ["packages/react/transform/crates/*"], which covers packages/react/transform/crates/swc_plugin_inject, so the new path dependency is acceptable.
Web Explorer#5393 Bundle Size — 365.48KiB (0%).cb88d37(current) vs e2993a0 main#5383(baseline) Bundle metrics
|
| Current #5393 |
Baseline #5383 |
|
|---|---|---|
145.65KiB |
145.65KiB |
|
32KiB |
32KiB |
|
0% |
0% |
|
8 |
8 |
|
8 |
8 |
|
220 |
220 |
|
16 |
16 |
|
3.37% |
3.37% |
|
4 |
4 |
|
0 |
0 |
Bundle size by type no changes
| Current #5393 |
Baseline #5383 |
|
|---|---|---|
239.46KiB |
239.46KiB |
|
94.02KiB |
94.02KiB |
|
32KiB |
32KiB |
Bundle analysis report Branch refactor/extract-swc-plugin-inje... Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#5400 Bundle Size — 237.66KiB (0%).cb88d37(current) vs e2993a0 main#5390(baseline) Bundle metrics
|
| Current #5400 |
Baseline #5390 |
|
|---|---|---|
0B |
0B |
|
0B |
0B |
|
0% |
0% |
|
0 |
0 |
|
4 |
4 |
|
165 |
165 |
|
67 |
67 |
|
46.76% |
46.76% |
|
2 |
2 |
|
0 |
0 |
Bundle size by type no changes
| Current #5400 |
Baseline #5390 |
|
|---|---|---|
145.76KiB |
145.76KiB |
|
91.91KiB |
91.91KiB |
Bundle analysis report Branch refactor/extract-swc-plugin-inje... Project dashboard
Generated by RelativeCI Documentation Report issue
CodSpeed Performance ReportMerging #1782 will improve performances by 14.82%Comparing Summary
Benchmarks breakdown
Footnotes
|
swc_plugin_directive_dce & swc_plugins_shared::cssswc_plugin_inject & swc_plugins_shared::css
colinaaa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the interface InjectVisitorConfig has been removed when running napi build. Maybe we should figure out why and how to fix later.
ad46506 to
cb88d37
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/react/transform/crates/swc_plugin_inject/lib.rs (1)
108-120: Use a plain Ident for the exported name in ImportNamed.private_ident! is meant for fresh locals; the exported name should be a plain Ident to avoid surprising hygiene. Minimal tweak:
- ImportSpecifier::Named(ImportNamedSpecifier { - span: DUMMY_SP, - local: ii.clone(), - imported: Some(ModuleExportName::Ident(private_ident!(imported.as_str()))), - is_type_only: Default::default(), - }), + ImportSpecifier::Named(ImportNamedSpecifier { + span: DUMMY_SP, + local: ii.clone(), + imported: Some(ModuleExportName::Ident(Ident::new(imported.clone().into(), DUMMY_SP))), + is_type_only: Default::default(), + }),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.changeset/silver-pandas-teach.md(1 hunks)packages/react/transform/Cargo.toml(1 hunks)packages/react/transform/crates/swc_plugin_inject/Cargo.toml(1 hunks)packages/react/transform/crates/swc_plugin_inject/lib.rs(2 hunks)packages/react/transform/crates/swc_plugin_inject/napi.rs(1 hunks)packages/react/transform/crates/swc_plugins_shared/Cargo.toml(1 hunks)packages/react/transform/crates/swc_plugins_shared/css.rs(1 hunks)packages/react/transform/crates/swc_plugins_shared/lib.rs(1 hunks)packages/react/transform/src/lib.rs(1 hunks)packages/react/transform/src/swc_plugin_compat_post/mod.rs(1 hunks)packages/react/transform/src/swc_plugin_snapshot/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/react/transform/Cargo.toml
- packages/react/transform/crates/swc_plugins_shared/css.rs
- packages/react/transform/crates/swc_plugins_shared/lib.rs
- packages/react/transform/src/swc_plugin_compat_post/mod.rs
- packages/react/transform/crates/swc_plugins_shared/Cargo.toml
- .changeset/silver-pandas-teach.md
- packages/react/transform/src/swc_plugin_snapshot/mod.rs
- packages/react/transform/crates/swc_plugin_inject/Cargo.toml
- packages/react/transform/crates/swc_plugin_inject/napi.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: gaoachao
PR: lynx-family/lynx-stack#1771
File: packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/basic_component_with_static_sibling.js:2-2
Timestamp: 2025-09-18T04:43:54.407Z
Learning: In the lynx-family/lynx-stack repository, the `add_pure_comment` function in packages/react/transform/src/swc_plugin_compat/mod.rs (around lines 478-482) is specifically for `wrapWithLynxComponent` calls, not `createSnapshot` calls. The PURE comment injection for `createSnapshot` is handled separately in swc_plugin_snapshot/mod.rs.
Learnt from: gaoachao
PR: lynx-family/lynx-stack#1714
File: packages/react/transform/Cargo.toml:19-19
Timestamp: 2025-09-10T11:42:36.855Z
Learning: In packages/react/transform/Cargo.toml, the crate uses serde derive macros (#[derive(Serialize, Deserialize)]) in multiple files including src/esbuild.rs and src/swc_plugin_extract_str/mod.rs, so the "derive" feature is required when migrating to workspace dependencies.
📚 Learning: 2025-09-10T11:42:36.855Z
Learnt from: gaoachao
PR: lynx-family/lynx-stack#1714
File: packages/react/transform/Cargo.toml:19-19
Timestamp: 2025-09-10T11:42:36.855Z
Learning: In packages/react/transform/Cargo.toml, the crate uses serde derive macros (#[derive(Serialize, Deserialize)]) in multiple files including src/esbuild.rs and src/swc_plugin_extract_str/mod.rs, so the "derive" feature is required when migrating to workspace dependencies.
Applied to files:
packages/react/transform/src/lib.rs
🧬 Code graph analysis (2)
packages/react/transform/crates/swc_plugin_inject/lib.rs (1)
packages/react/transform/index.d.ts (1)
InjectVisitorConfig(461-463)
packages/react/transform/src/lib.rs (1)
packages/react/transform/index.d.ts (1)
InjectVisitorConfig(461-463)
🔇 Additional comments (3)
packages/react/transform/crates/swc_plugin_inject/lib.rs (2)
14-15: napi module extraction is a clean boundary.Nice split between core logic and Node bindings.
189-192: Tests reference core types via crate root — good decoupling from N-API.Keeps unit tests independent of binding layer.
packages/react/transform/src/lib.rs (1)
64-64: Confirm swc_plugin_inject::napi import and TS interface
- packages/react/transform/index.d.ts contains InjectVisitorConfig with the expected inject: Record<...> shape (lines ~461–463).
- Ripgrep skipped Rust files so remaining Rust call sites importing from the crate root couldn't be verified. Re-run locally and report matches:
rg -n "swc_plugin_inject::" -g '!/target/' -S | rg -v "::napi"
Summary by CodeRabbit
Refactor
Chores
Documentation
Checklist