perf: unify route bindings in constructors - #8074
Conversation
|
View your CI Pipeline Execution ↗ for commit e027650
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughThe React, Solid, and Vue routers now install route APIs through shared initializers. Route-specific IDs and paths remain bound. Tests verify that route and root route instances own every API member. A patch changeset covers all three packages. ChangesUnified route API bindings
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The constructor-based route-binding refactor is localized across the three adapters, but the initializer target typing may reject adapter-specific members under strict TypeScript, potentially causing build or type-check failures. This is currently unverified and is not a merge blocker, but it should receive explicit owner follow-up. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Bundle Size Benchmarks
The following scenarios have bundle-size changes compared with the baseline:
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
🚀 Changeset Version Preview4 package(s) bumped directly, 19 bumped as dependents. 🟩 Patch bumps
|
There was a problem hiding this comment.
🔇 Additional comments (5)
packages/react-router/src/route.tsx (1)
508-549: 📐 Maintainability & Code Quality
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the initializer target type.
Each initializer accepts
AnyRoutebut writes adapter-specificuse*,useNavigate, andLinkmembers. IfAnyRouteresolves to the core route type, strict TypeScript rejects these property assignments. Use an adapter-specific target interface or intersection type that declares the assigned members.
packages/react-router/src/route.tsx#L508-L549: type the initializer target with the React route API members.packages/solid-router/src/route.tsx#L466-L499: type the initializer target with the Solid route API members.packages/vue-router/src/route.ts#L472-L509: type the initializer target with the Vue route API members.As per coding guidelines,
**/*.{ts,tsx}must use TypeScript strict mode with extensive type safety.packages/react-router/tests/route.test.tsx (1)
79-86: LGTM!.changeset/easy-streets-stare.md (1)
1-7: LGTM!packages/solid-router/tests/route.test.tsx (1)
78-85: LGTM!packages/vue-router/tests/route.test.tsx (1)
78-85: LGTM!
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c5d945d0-33f1-4ce1-ae9d-6490d8c92e50
📒 Files selected for processing (7)
.changeset/easy-streets-stare.mdpackages/react-router/src/route.tsxpackages/react-router/tests/route.test.tsxpackages/solid-router/src/route.tsxpackages/solid-router/tests/route.test.tsxpackages/vue-router/src/route.tspackages/vue-router/tests/route.test.tsx
Merging this PR will regress 5 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem client unique-location-churn (solid) |
325.9 KB | 348.1 KB | -6.38% |
| ❌ | Memory | mem server error-paths redirect (react) |
303.6 KB | 323.1 KB | -6.02% |
| ❌ | Memory | mem server peak-large-page (vue) |
1,010.6 KB | 1,058.7 KB | -4.54% |
| ❌ | Memory | mem server request-churn (solid) |
728.8 KB | 762.9 KB | -4.47% |
| ❌ | Memory | mem server error-paths unmatched (react) |
415.8 KB | 433.7 KB | -4.13% |
| ⚡ | Memory | mem client navigation-churn (vue) |
1.7 MB | 1.6 MB | +6.63% |
| ⚡ | Simulation | client-search-params navigation loop (react) |
177 ms | 168.4 ms | +5.11% |
| ⚡ | Memory | mem server server-fn-churn (solid) |
360.3 KB | 345.1 KB | +4.4% |
| ⚡ | Memory | mem server aborted-requests (solid) |
1.3 MB | 1.2 MB | +4.31% |
| ⚡ | Memory | mem server server-fn-churn (react) |
395.7 KB | 381 KB | +3.87% |
| ⚡ | Memory | mem server error-paths not-found (solid) |
584.7 KB | 563.9 KB | +3.69% |
| 👁 | Memory | mem server error-paths not-found (vue) |
2,190.4 KB | 503.6 KB | ×4.3 |
| 👁 | Memory | mem server error-paths not-found (react) |
373.4 KB | 411.7 KB | -9.3% |
| 👁 | Memory | mem client unique-location-churn (vue) |
487.9 KB | 593.6 KB | -17.8% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing perf-all-adapters-unify-route-bindings (e027650) with main (bdaf73a)
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud has identified a possible root cause for your failed CI:
We classified this failure as an environment issue rather than a code change because the error — a version mismatch between @rspack/core@2.0.8 and @rspack/binding@2.0.0 — is unrelated to the route-binding refactor in this PR. The failing project (tanstack-start-example-basic-rsbuild) was not touched by our changes, and no lockfile or package configuration was modified. Reinstalling dependencies to align the rspack package versions should resolve the build failure.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
Summary by CodeRabbit
Improvements
Linkbehavior.Bug Fixes
Tests