-
Notifications
You must be signed in to change notification settings - Fork 111
refactor(react/transfrom): migrate to Cargo workspace #1714
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: 66b7630 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 |
📝 WalkthroughWalkthroughCentralizes crate versions in a new [workspace.dependencies] in the root Cargo.toml, switches several crate Cargo.toml files to workspace-managed dependencies, enables LTO and size optimization in release profile, adds a wasm getrandom feature, and adds an empty changeset metadata file. No runtime or public API changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
👍 We may also add a CI check to enforce using workspace version. See: https://github.com/web-infra-dev/rspack/blob/6aef3a3c7347265eec9ccc505307c2f2f3ca04c3/xtask/src/deny_ext/mod.rs#L24
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 (1)
Cargo.toml (1)
10-26: Centralizing versions is good; watch out for dropped per-crate features.Workspace deps don’t propagate features. If any member needs
serde/derive(very common), ensure that crate re-adds the feature locally.Would you like me to scan members and open a follow-up PR to add missing features where needed?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/fluffy-cougars-post.md(1 hunks)Cargo.toml(1 hunks)packages/react/transform/Cargo.toml(1 hunks)packages/web-platform/web-style-transformer/Cargo.toml(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/fluffy-cougars-post.md
🧠 Learnings (2)
📚 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/fluffy-cougars-post.md
📚 Learning: 2025-07-22T09:26:16.722Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:26:16.722Z
Learning: In the lynx-family/lynx-stack repository, CI checks require changesets when files matching the pattern "src/**" are modified (as configured in .changeset/config.json). For internal changes that don't need meaningful changesets, an empty changeset file is used to satisfy the CI requirement while not generating any release notes.
Applied to files:
.changeset/fluffy-cougars-post.md
⏰ 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). (4)
- GitHub Check: Validate PR title
- GitHub Check: build / Build (Windows)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: test-rust / Test (Ubuntu)
🔇 Additional comments (4)
.changeset/fluffy-cougars-post.md (1)
1-3: Empty changeset aligns with repo policy for internal refactors.Per retrieved learnings for this repo, an empty changeset is acceptable to satisfy CI for non-user-facing refactors. Please just confirm CI accepts this file as-is.
Note: I used the stored learnings you provided to validate this behavior.
packages/web-platform/web-style-transformer/Cargo.toml (1)
11-13: LGTM: moved to workspace-managed wasm deps.
js-sys,lazy_static, andwasm-bindgennow resolve via workspace; versions will stay consistent across crates.packages/react/transform/Cargo.toml (2)
22-22: SWC feature set preserved — good.Feature list matches prior intent while moving to workspace-managed versioning.
26-27: Ignore WASM RNG feature suggestionThe custom
getrandomimplementation is registered viaregister_custom_getrandom!(custom_getrandom)inpackages/react/transform/src/lib.rs, so keepingfeatures = ["custom"]is correct.Likely an incorrect or invalid review comment.
React Example#5073 Bundle Size — 238.2KiB (0%).66b7630(current) vs 05c21dc main#5065(baseline) Bundle metrics
|
| Current #5073 |
Baseline #5065 |
|
|---|---|---|
0B |
0B |
|
0B |
0B |
|
0% |
0% |
|
0 |
0 |
|
4 |
4 |
|
163 |
163 |
|
67 |
67 |
|
46.88% |
46.88% |
|
2 |
2 |
|
0 |
0 |
Bundle size by type no changes
| Current #5073 |
Baseline #5065 |
|
|---|---|---|
145.76KiB |
145.76KiB |
|
92.45KiB |
92.45KiB |
Bundle analysis report Branch refactor/cargo-workspace Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#5066 Bundle Size — 361.93KiB (0%).66b7630(current) vs 05c21dc main#5058(baseline) Bundle metrics
|
| Current #5066 |
Baseline #5058 |
|
|---|---|---|
144.4KiB |
144.4KiB |
|
31.84KiB |
31.84KiB |
|
0% |
0% |
|
8 |
8 |
|
8 |
8 |
|
219 |
219 |
|
16 |
16 |
|
3.34% |
3.34% |
|
4 |
4 |
|
0 |
0 |
Bundle size by type no changes
| Current #5066 |
Baseline #5058 |
|
|---|---|---|
236.07KiB |
236.07KiB |
|
94.02KiB |
94.02KiB |
|
31.84KiB |
31.84KiB |
Bundle analysis report Branch refactor/cargo-workspace Project dashboard
Generated by RelativeCI Documentation Report issue
CodSpeed Performance ReportMerging #1714 will not alter performanceComparing 🎉 Hooray!
|
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
♻️ Duplicate comments (1)
packages/react/transform/Cargo.toml (1)
19-19: Thank you for preserving serde derive.Keeping
features = ["derive"]avoids macro resolution failures after the workspace switch. This addresses the earlier feedback.
🧹 Nitpick comments (1)
packages/react/transform/Cargo.toml (1)
22-22: Consider gating internal/testing swc_core features.
"__testing_transform"(and sometimes"__visit") are internal/testing aids. If not required at runtime, remove them from default features or gate behind a local cargo feature to speed builds.Example:
-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_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"] }Or gate testing-only ones:
[features] noop = [] testing = [] [target.'cfg(any())'.dependencies] # none [dependencies] # swc_core as above without testing flags [dev-dependencies] swc_core = { workspace = true, features = ["__testing_transform"] }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Cargo.toml(1 hunks)packages/react/transform/Cargo.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: gaoachao
PR: lynx-family/lynx-stack#1714
File: packages/react/transform/Cargo.toml:19-19
Timestamp: 2025-09-10T11:42:36.812Z
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.812Z
Learnt from: gaoachao
PR: lynx-family/lynx-stack#1714
File: packages/react/transform/Cargo.toml:19-19
Timestamp: 2025-09-10T11:42:36.812Z
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/Cargo.toml
🔇 Additional comments (2)
packages/react/transform/Cargo.toml (2)
11-13: Workspace migration for shared crates looks good.Converting convert_case, hex, indexmap, once_cell, regex, rustc-hash, sha-1, and version-compare to workspace-managed deps is consistent and reduces version drift.
Also applies to: 16-18, 21-21, 23-23
25-27: No changes needed—custom getrandom registration is in place
The wasm32-unknown-unknown module already callsregister_custom_getrandom!(custom_getrandom), satisfying the “custom” feature requirement.
Partial cherry-pick from #1458
Summary by CodeRabbit
Performance
Chores
Notes
Checklist