feat(start-core): add method and serverFnMeta property to ServerFnCtx
#6394
Conversation
📝 WalkthroughWalkthroughHandlers, runtime wiring, and types now accept and propagate an HTTP Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Browser/Client
participant Runtime as ServerFunctionsHandler\n(packages/start-server-core)
participant Fn as Server Function\n(factory/route)
participant InnerFn as Inner Server Function\n(optional)
Client->>Runtime: invoke server function (GET or POST)
Runtime->>Fn: call handler({ context, method, serverFnMeta })
alt handler calls inner server fn
Fn->>InnerFn: await innerFn() (method propagated)
InnerFn-->>Fn: return inner result (includes method)
end
Fn-->>Runtime: return payload (includes name, context, method, serverFnMeta, inner?)
Runtime-->>Client: response with payload (method + serverFnMeta)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
Comment |
|
View your CI Pipeline Execution ↗ for commit 8b069fc
☁️ Nx Cloud last updated this comment at |
hokkyss
changed the title
hokkyss
force-pushed
the
server-fn-method
branch
from
3d7c5b8 to
ddda24a
Compare
|
The three tests related to this PR's changes are successful. but this is confusing... |
hokkyss
changed the title
hokkyss
changed the title
serverFnMeta property to ServerFnCtx
hokkyss
changed the title
serverFnMeta property to ServerFnCtxserverFnMeta property to ServerFnCtx
hokkyss
force-pushed
the
server-fn-method
branch
from
0b83a88 to
db9e525
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@e2e/react-start/server-functions/src/routes/function-metadata/index.tsx`:
- Line 26: The data-testid on the returned div in the function-metadata route is
misleadingly set to "method-route-component"; update the JSX in the component
that returns the div (the element producing data-testid) to a clearer identifier
reflecting this file/route (e.g., "function-metadata-route-component" or
"function-metadata-component") so tests and readers map the DOM to the correct
route/component.
In `@e2e/solid-start/server-functions/src/routes/function-metadata/index.tsx`:
- Line 26: Rename the data-testid on the root div of the metadata route from
"method-route-component" to "metadata-route-component" (update the JSX attribute
in the return statement) and then update any tests or selectors that reference
"method-route-component" to use "metadata-route-component" so the test id
consistently reflects the metadata route.
In `@e2e/solid-start/server-functions/tests/server-functions.spec.ts`:
- Around line 506-533: The test currently iterates over buttons returned from
page.locator('[data-testid^="btn-fn-"]').elementHandles() but never asserts that
any buttons were found, allowing a false-positive when zero buttons render;
amend the test by adding an assertion right after obtaining buttons (e.g.,
assert buttons.length > 0 or use expect(buttons.length).toBeGreaterThan(0)) to
ensure the scenario is exercised before the for...of loop that uses testId,
suffix, expected, and the click/assertions on fn-result-... and
fn-comparison-... elements.
In `@e2e/vue-start/server-functions/src/routes/function-metadata/index.tsx`:
- Line 11: The data-testid on the route component is named
"method-route-component" but should be "metadata-route-component" to match the
route purpose and the Solid version; update the JSX return in this route (the
component that returns <div class="p-2 m-2 grid gap-2" ...>) to use
data-testid="metadata-route-component" so tests and naming are consistent.
- Around line 1-4: Reorder the imports so that external library imports come
before local imports: place the "defineComponent" import from "vue" above the
local relative imports (the modules that export getServerFn, postServerFn,
getServerFnCallingServerFn, postServerFnCallingServerFn) while keeping the
"@tanstack/vue-router" import (createFileRoute) at the top; ensure the final
import order is external packages first (e.g., '@tanstack/vue-router', 'vue')
then local './-functions/...' imports to satisfy ESLint import-order rules.
In `@e2e/vue-start/server-functions/src/routes/function-method/index.tsx`:
- Line 60: Remove or populate the empty <h2 class="font-bold text-lg"></h2>
element in the JSX: either delete this tag if no heading is needed, or replace
its inner content with a meaningful title string (e.g., "Server Functions" or
the appropriate section heading) so the heading is not empty; update the JSX
return where the <h2 class="font-bold text-lg"> appears.
- Around line 5-26: The object literal "functions" (containing
getServerFnCallingPost and postServerFnCallingGet) has mixed indentation on
lines around the expected blocks; normalize indentation to the project's
space-based style (no tabs, consistent number of spaces) so keys and nested
properties (name, method, innerFnResult) align with other entries and the
satisfies Record<string, TestCase> declaration—update getServerFnCallingPost and
postServerFnCallingGet blocks to use the same space indentation as the rest of
the file.
- Around line 1-3: Reorder and adjust imports so external library imports remain
first, then Vue, then local modules: import Link, createFileRoute, deepEqual
from '@tanstack/vue-router'; import computed, defineComponent, ref from 'vue'
and use a type-only import for PropType (import type { PropType } from 'vue');
finally import getServerFnCallingPost and postServerFnCallingGet from
'./-functions/serverFnCallingServerFn' — update the import statements
referencing PropType, computed, defineComponent, ref, Link, createFileRoute,
deepEqual, getServerFnCallingPost, and postServerFnCallingGet accordingly.
🧹 Nitpick comments (11)
e2e/solid-start/server-functions/src/routes/function-method/index.tsx (3)
9-30: Inconsistent indentation in test case definitions.Lines 13, 20, 23, 37-38 appear to use tabs while the rest of the file uses spaces, causing visual misalignment. This should be normalized for consistency.
66-68: Empty<h2>element serves no purpose.The
<h2>tag on line 68 has no content. Consider removing it or adding a meaningful heading (e.g., the test name).- <h2 className="font-bold text-lg"></h2> + <h2 className="font-bold text-lg">{expected.name}</h2>
47-52: Consider stronger typing forTestCaseinterface.The interface uses
anytypes which reduces type safety. Since this is test code, it's acceptable, but could be improved:interface TestCase { fn: () => Promise<{ name: string; method: string; innerFnResult: { method: string } }> expected: { name: string; method: string; innerFnResult: { method: string } } }e2e/react-start/server-functions/src/routes/function-method/index.tsx (2)
9-30: Inconsistent indentation in test case definitions.Same issue as the Solid version - lines 13, 20, 23, 37-38 appear to use tabs while other lines use spaces.
64-66: Empty<h2>element serves no purpose.Consider removing or populating with the test name for better UX:
- <h2 className="font-bold text-lg"></h2> + <h2 className="font-bold text-lg">{expected.name}</h2>e2e/solid-start/server-functions/src/routes/function-metadata/index.tsx (1)
7-19: Consider usingPromise.allfor concurrent fetching.The loader awaits each server function sequentially. Since these calls are independent, you can parallelize them for better performance:
♻️ Suggested refactor
loader: async () => { - const normalGet = await getServerFn(); - const normalPost = await postServerFn(); - const nestingGet = await getServerFnCallingServerFn(); - const nestingPost = await postServerFnCallingServerFn(); + const [normalGet, normalPost, nestingGet, nestingPost] = await Promise.all([ + getServerFn(), + postServerFn(), + getServerFnCallingServerFn(), + postServerFnCallingServerFn(), + ]); return { normalGet, normalPost, nestingGet, nestingPost, } }e2e/vue-start/server-functions/src/routes/function-metadata/index.tsx (1)
51-62: Consider usingPromise.allfor concurrent fetching.Same as the Solid version—these independent calls can be parallelized:
♻️ Suggested refactor
loader: async () => { - const normalGet = await getServerFn(); - const normalPost = await postServerFn(); - const nestingGet = await getServerFnCallingServerFn(); - const nestingPost = await postServerFnCallingServerFn(); + const [normalGet, normalPost, nestingGet, nestingPost] = await Promise.all([ + getServerFn(), + postServerFn(), + getServerFnCallingServerFn(), + postServerFnCallingServerFn(), + ]); return { normalGet, normalPost, nestingGet, nestingPost, } }e2e/vue-start/server-functions/src/routes/function-method/index.tsx (1)
85-89: Consider adding error handling for the async call.If the server function call rejects, the UI will remain in the "Loading..." state with no feedback to the user or test runner.
♻️ Suggested improvement
onClick={() => { props.fn().then((data) => { result.value = data + }).catch((error) => { + result.value = { error: String(error) } }) }}e2e/react-start/server-functions/tests/server-functions.spec.ts (1)
921-1019: Comprehensive serverFnMeta validation.The test thoroughly validates:
- Normal server functions receive complete metadata (
id,name,filename)- Nested server function calls preserve metadata correctly
- All metadata fields are non-empty strings
The use of
toHavePropertywith dot notation for nested paths is appropriate for validating the nested metadata structure.Consider extracting the repeated metadata validation logic into a helper function to reduce duplication. For example:
♻️ Optional refactor to reduce duplication
function expectValidMetadata(metadata: Record<string, unknown>, prefix = '') { const path = prefix ? `${prefix}.` : ''; expect(metadata).toHaveProperty(`${path}id`); expect(metadata).toHaveProperty(`${path}name`); expect(metadata).toHaveProperty(`${path}filename`); expect(typeof (prefix ? metadata[prefix.split('.')[0]] : metadata).id).toBe('string'); // ... similar for other fields }e2e/vue-start/server-functions/tests/server-functions.spec.ts (1)
534-632: Add explicit non-null assertions before JSON.parse for clearer failures.Right now a null
textContent()will throw a JSON parse error; a targeted expectation makes failures easier to diagnose.♻️ Suggested tweak
const loaderNestingGet = await page .getByTestId('loader-nesting-get-function-metadata') .textContent() const loaderNestingPost = await page .getByTestId('loader-nesting-post-function-metadata') .textContent() + expect(loaderNestingGet).toBeTruthy() + expect(loaderNestingPost).toBeTruthy() + // metadata should have `id`, `name`, and `filename` property, with all of them being a non-empty string const nestingGetMetadata = JSON.parse(loaderNestingGet!) expect(nestingGetMetadata).toHaveProperty('meta.id')e2e/solid-start/server-functions/tests/server-functions.spec.ts (1)
578-633: Guard JSON parsing to fail with clearer intent.If
textContent()returns an empty string,JSON.parse('')throws a generic error. An explicit truthy check makes failures clearer and easier to debug.✅ Suggested fix
const loaderNestingGet = await page .getByTestId('loader-nesting-get-function-metadata') .textContent() const loaderNestingPost = await page .getByTestId('loader-nesting-post-function-metadata') .textContent() + expect(loaderNestingGet).toBeTruthy() + expect(loaderNestingPost).toBeTruthy()
e2e/react-start/server-functions/src/routes/function-metadata/index.tsx
Outdated
Show resolved
Hide resolved
e2e/solid-start/server-functions/src/routes/function-metadata/index.tsx
Outdated
Show resolved
Hide resolved
e2e/vue-start/server-functions/src/routes/function-metadata/index.tsx
Outdated
Show resolved
Hide resolved
e2e/vue-start/server-functions/src/routes/function-metadata/index.tsx
Outdated
Show resolved
Hide resolved
e2e/vue-start/server-functions/src/routes/function-method/index.tsx
Outdated
Show resolved
Hide resolved
e2e/vue-start/server-functions/src/routes/function-method/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@e2e/react-start/server-functions/src/routes/function-method/index.tsx`:
- Around line 66-70: Remove the empty heading element (<h2 className="font-bold
text-lg"></h2>) inside the component rendering the test card (the div with
data-testid={`test-${expected.name}`}); either delete this empty <h2> to avoid
accessibility/noise or replace it with meaningful content (e.g., a title derived
from expected.name) so the element is not empty.
- Around line 93-95: The onClick handler calls fn() and only handles the
fulfilled case (fn().then(setResult)), which leaves rejections as unhandled
promises; update the onClick callback (the function passed to onClick) to handle
failures by adding a .catch(...) (or convert to async/await with try/catch) and
surface the error to the UI—e.g., call setResult with an error object/string or
call a new setError state updater—so both fn() success (setResult) and rejection
paths are handled.
♻️ Duplicate comments (2)
e2e/vue-start/server-functions/src/routes/function-method/index.tsx (2)
1-6: Fix import ordering and use type-only import forPropType.The
vueimport should occur before the local import, andPropTypeis only used as a type so it should use a type-only import syntax.🔧 Suggested fix
import { Link, createFileRoute, deepEqual } from '@tanstack/vue-router' +import { computed, defineComponent, ref } from 'vue' +import type { PropType } from 'vue' import { getServerFnCallingPost, postServerFnCallingGet, } from './-functions/serverFnCallingServerFn' -import { PropType, computed, defineComponent, ref } from 'vue'
61-61: Empty heading element.The
<h2>element has no content. Either add a meaningful heading (e.g.,{props.expected.name}) or remove it.🔧 Suggested fix
- <h2 class="font-bold text-lg"></h2> + <h2 class="font-bold text-lg">{props.expected.name}</h2>
🧹 Nitpick comments (6)
e2e/vue-start/server-functions/src/routes/function-metadata/-functions/serverFnCallingServerFn.ts (1)
19-31: Consider:postServerFnCallingServerFnuses default GET method.Both
getServerFnCallingServerFnandpostServerFnCallingServerFnuse the default GET method. If the intent is to test a POST server function calling other server functions, consider adding{ method: 'POST' }:-export const postServerFnCallingServerFn = createServerFn().handler( +export const postServerFnCallingServerFn = createServerFn({ method: 'POST' }).handler(However, this pattern is consistent with the React and Solid implementations, so if the current behavior is intentional for your test scenario, feel free to disregard.
e2e/vue-start/server-functions/src/routes/function-method/index.tsx (2)
8-34: MoveTestCaseinterface before its usage.The
TestCaseinterface is referenced on line 29 but defined on lines 31-34. While TypeScript hoists interface declarations, placing the interface definition before its usage improves readability.♻️ Suggested reorder
+interface TestCase { + fn: () => Promise<any> + expected: any +} + const functions = { getServerFnCallingPost: { fn: getServerFnCallingPost, expected: { name: 'getServerFnCallingPost', method: 'GET', innerFnResult: { method: 'POST', }, }, }, postServerFnCallingGet: { fn: postServerFnCallingGet, expected: { name: 'postServerFnCallingGet', method: 'POST', innerFnResult: { method: 'GET', }, }, }, } satisfies Record<string, TestCase> - -interface TestCase { - fn: () => Promise<any> - expected: any -}
84-88: Consider adding error handling for the async call.The promise chain lacks a
.catch()handler. If the server function fails, the rejection will be unhandled, making test failures harder to debug.♻️ Suggested improvement
onClick={() => { - props.fn().then((data) => { - result.value = data - }) + props.fn() + .then((data) => { + result.value = data + }) + .catch((err) => { + console.error('Server function error:', err) + result.value = { error: String(err) } + }) }}e2e/solid-start/server-functions/src/routes/function-metadata/index.tsx (1)
10-22: Consider usingPromise.allfor parallel fetching.The loader awaits each server function sequentially. For E2E tests this is fine, but for better performance patterns, parallel fetching could be used:
loader: async () => { - const normalGet = await getServerFn() - const normalPost = await postServerFn() - const nestingGet = await getServerFnCallingServerFn() - const nestingPost = await postServerFnCallingServerFn() + const [normalGet, normalPost, nestingGet, nestingPost] = await Promise.all([ + getServerFn(), + postServerFn(), + getServerFnCallingServerFn(), + postServerFnCallingServerFn(), + ]) return { normalGet, normalPost, nestingGet, nestingPost, } },packages/start-client-core/src/tests/createServerFn.test-d.ts (1)
414-428: Consider addingmethodandserverFnMetaassertions for consistency.This test case uses
createServerFn()without an explicit method and doesn't includemethodorserverFnMetain the handler options assertion. While the test focuses on data/context overriding behavior, adding these properties would maintain consistency with other test cases.♻️ Suggested change
createServerFn() .middleware([middleware2]) .inputValidator( () => ({ input: 'c', }) as const, ) - .handler(({ data, context }) => { + .handler(({ data, context, method, serverFnMeta }) => { expectTypeOf(data).toEqualTypeOf<{ readonly input: 'c' }>() expectTypeOf(context).toEqualTypeOf<{ readonly context: 'bb' }>() + expectTypeOf(method).toEqualTypeOf<'GET'>() + expectTypeOf(serverFnMeta).toEqualTypeOf<ServerFnMeta>() })e2e/react-start/server-functions/src/routes/function-method/index.tsx (1)
51-54: Avoidanyin TestCase for strict TS.
Replaceanywith a concrete shape to preserve type safety and align with strict TS usage. As per coding guidelines, ...♻️ Suggested typing
-interface TestCase { - fn: () => Promise<any> - expected: any -} +interface ServerFnResult { + name: string + method: 'GET' | 'POST' + innerFnResult: { + method: 'GET' | 'POST' + } +} + +interface TestCase { + fn: () => Promise<ServerFnResult> + expected: ServerFnResult +}
e2e/react-start/server-functions/src/routes/function-method/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@e2e/vue-start/server-functions/src/routes/function-method/index.tsx`:
- Around line 84-88: The onClick handler currently calls props.fn() without
handling rejections, so wrap the call to props.fn() with proper error handling
(either convert the inline callback to an async function using try/catch or add
a .catch handler to the promise) and ensure you update the UI state on failure
(e.g., set result.value to an error message or set an error state and stop any
loading indicator). Update the onClick callback that calls props.fn() and the
code that sets result.value so both success and failure paths update state and
optionally log the error (console.error or a logger) for debugging.
🧹 Nitpick comments (1)
e2e/vue-start/server-functions/src/routes/function-method/index.tsx (1)
30-35: Consider movingTestCaseinterface before its usage.The interface is defined after it's referenced in the
satisfiesclause. While TypeScript hoists interfaces, placing the definition before usage (before line 9) improves readability.♻️ Suggested reorder
+interface TestCase { + fn: () => Promise<any> + expected: any +} + const functions = { getServerFnCallingPost: { ... }, } satisfies Record<string, TestCase> - -interface TestCase { - fn: () => Promise<any> - expected: any -}
| const params = { | ||
| context, | ||
| data: formData, | ||
| method, |
There was a problem hiding this comment.
method must be uppercase, is it guaranteed?
There was a problem hiding this comment.
applies to payload.method as well
There was a problem hiding this comment.
Looking at https://developer.mozilla.org/en-US/docs/Web/API/Request/method, it should be uppercase, considering we are using web request (with srvx's help).
Lemme uppercase it just in case
2 participants
Add
methodandserverFnMetaproperty toServerFnCtxtypeSummary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.