[feat]: Add stable IDs for server functions#7693
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR adds optional ChangesManual server-function ID reservation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
View your CI Pipeline Execution ↗ for commit e58444b
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview2 package(s) bumped directly, 12 bumped as dependents. 🟨 Minor bumps
🟩 Patch bumps
|
Bundle Size Benchmarks
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. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/start-plugin-core/src/start-compiler/compiler.ts`:
- Around line 674-689: The reserveFunctionId logic currently only checks
reserved/generated IDs and manual owners, so a manual server function ID can
still collide with a known server function discovered by getKnownServerFns().
Update reserveFunctionId in compiler.ts to also consult the known
server-function set before accepting opts.functionId, and reject any manual ID
that matches an existing server function ID so handleCreateServerFn never reuses
an unrelated knownFns entry.
In `@packages/start-plugin-core/src/start-compiler/handleCreateServerFn.ts`:
- Around line 184-215: Resolve static template-literal IDs in the createServerFn
ID extraction path: handle no-expression template literals the same way as
string literals so `resolveStaticString`-compatible values like `id:
\`get-user\`` and `const id = \`get-user\`; createServerFn({ id })` don’t fall
through to the generic error. Update the ID resolution logic in
`handleCreateServerFn` (especially the branches that currently check
`t.isStringLiteral`, `t.isIdentifier`, and `bindingInit`) to accept the static
template-literal case and still validate it against
`MANUAL_SERVER_FN_ID_PATTERN` before returning the resolved value.
- Around line 227-250: The helper getCreateServerFnCallExpression only matches
an innermost callee named createServerFn, so aliased or namespace-resolved
factory calls are missed and manual ids are skipped. Update the resolution logic
in getCreateServerFnCallExpression (and any related createServerFn candidate
handling in handleCreateServerFn.ts) to unwrap aliased/namespace call chains and
recognize the actual factory call regardless of local import name, so { id:
"..." } is preserved instead of falling back to generated IDs.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01e9cb30-43da-4380-bb1e-cd0ea1f81bcd
📒 Files selected for processing (6)
packages/start-client-core/src/createServerFn.tspackages/start-client-core/src/tests/createServerFn.test-d.tspackages/start-plugin-core/src/start-compiler/compiler.tspackages/start-plugin-core/src/start-compiler/handleCreateServerFn.tspackages/start-plugin-core/src/start-compiler/types.tspackages/start-plugin-core/tests/createServerFn/createServerFn.test.ts
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem server error-paths not-found (vue) |
387.9 KB | 678.7 KB | -42.84% |
| ❌ | Memory | mem server error-paths not-found (solid) |
427 KB | 705.5 KB | -39.48% |
| ❌ | Memory | mem server streaming-peak chunked (solid) |
30.3 MB | 33 MB | -8.29% |
| ❌ | Memory | mem server aborted-requests (vue) |
453.7 KB | 483.7 KB | -6.2% |
| ❌ | Simulation | client-route-tree-scale navigation loop (react) |
74.1 ms | 77.2 ms | -4.02% |
| ⚡ | Memory | mem server error-paths unmatched (react) |
606.9 KB | 264.4 KB | ×2.3 |
| ⚡ | Memory | mem server streaming-peak chunked (vue) |
13.9 MB | 11.1 MB | +25.48% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing stable-serverfn-ids-2 (e58444b) with main (a3e24c3)1
Footnotes
…tation on ID collisions
…hance tests for ID handling
…for manual server function IDs
…outer into stable-serverfn-ids-2
There was a problem hiding this comment.
Nx Cloud has identified a possible root cause for your failed CI:
We investigated the three failing tasks and determined none are caused by this PR's changes. The failures stem from pre-existing environment issues: stale build artifacts in a dist directory, a hardcoded port conflict between two Vue e2e services, and an incompatible nitro-nightly package that doesn't export the required subpath. All failing projects are outside the scope of our touched projects, so no code changes are needed.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
Summary by CodeRabbit
idwhen creating server functions for stable identifier-based runtime lookup, including through chaining and locally named re-exports.idshapes, and enforces collision rules for manual/manual and manual/generated cases.