-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
refactor: only handle createServerFn #6226
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,9 +9,19 @@ import { | |
| import { fromJSON, toCrossJSONAsync, toCrossJSONStream } from 'seroval' | ||
| import { getResponse } from './request-response' | ||
| import { getServerFnById } from './getServerFnById' | ||
| import type { Plugin as SerovalPlugin } from 'seroval' | ||
|
|
||
| let regex: RegExp | undefined = undefined | ||
|
|
||
| // Cache serovalPlugins at module level to avoid repeated calls | ||
| let serovalPlugins: Array<SerovalPlugin<any, any>> | undefined = undefined | ||
|
|
||
| // Known FormData 'Content-Type' header values - module-level constant | ||
| const FORM_DATA_CONTENT_TYPES = [ | ||
| 'multipart/form-data', | ||
| 'application/x-www-form-urlencoded', | ||
| ] | ||
|
Comment on lines
+12
to
+23
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Find the implementation of getDefaultSerovalPlugins
rg -n 'getDefaultSerovalPlugins' -t ts -t tsx -A 15
# Also check where seroval is imported from
rg -n 'from.*seroval|import.*seroval' -t ts -t tsx
# Check the file structure and surrounding code
cat -n packages/start-server-core/src/server-functions-handler.ts | head -50Repository: TanStack/router Length of output: 2064 🏁 Script executed: #!/bin/bash
# Find where serovalPlugins is initialized/assigned
rg -n 'serovalPlugins\s*=' packages/start-server-core/src/server-functions-handler.ts -A 3 -B 1
# Look for more context in the file, particularly around lines 57-60 mentioned in review
sed -n '50,75p' packages/start-server-core/src/server-functions-handler.ts
# Search for getDefaultSerovalPlugins in the client-core package
find packages/start-client-core -name '*.ts' -o -name '*.tsx' | head -20
# Look for any documentation or type definitions for SerovalPlugin
rg -n 'SerovalPlugin' packages/start-client-core -t ts -A 2 -B 1 | head -40Repository: TanStack/router Length of output: 4979 🏁 Script executed: #!/bin/bash
# Check the actual seroval Plugin type definition
rg -n 'Plugin' node_modules/seroval/package.json 2>/dev/null || echo "Checking alternative"
# Look for the actual type parameters being used in the parsePayload function
sed -n '65,75p' packages/start-server-core/src/server-functions-handler.ts
# Check if there are any other type safety issues in the file
rg -n ': any' packages/start-server-core/src/server-functions-handler.ts
# Look at how parsePayload uses the plugins
sed -n '65,95p' packages/start-server-core/src/server-functions-handler.tsRepository: TanStack/router Length of output: 1841 Module-level caching is correct, but TypeScript strict mode compliance is violated throughout the file. The lazy initialization and caching of
Replace all 🤖 Prompt for AI Agents |
||
|
|
||
| export const handleServerAction = async ({ | ||
| request, | ||
| context, | ||
|
|
@@ -29,34 +39,27 @@ export const handleServerAction = async ({ | |
| } | ||
|
|
||
| const method = request.method | ||
| const methodLower = method.toLowerCase() | ||
| const url = new URL(request.url, 'http://localhost:3000') | ||
| // extract the serverFnId from the url as host/_serverFn/:serverFnId | ||
| // Define a regex to match the path and extract the :thing part | ||
|
|
||
| // Execute the regex | ||
| const match = url.pathname.match(regex) | ||
| const serverFnId = match ? match[1] : null | ||
| const search = Object.fromEntries(url.searchParams.entries()) as { | ||
| payload?: any | ||
| createServerFn?: boolean | ||
| } | ||
|
|
||
| const isCreateServerFn = 'createServerFn' in search | ||
|
|
||
| if (typeof serverFnId !== 'string') { | ||
| throw new Error('Invalid server action param for serverFnId: ' + serverFnId) | ||
| } | ||
|
|
||
| const action = await getServerFnById(serverFnId, { fromClient: true }) | ||
|
|
||
| // Known FormData 'Content-Type' header values | ||
| const formDataContentTypes = [ | ||
| 'multipart/form-data', | ||
| 'application/x-www-form-urlencoded', | ||
| ] | ||
| // Initialize serovalPlugins lazily (cached at module level) | ||
| if (!serovalPlugins) { | ||
| serovalPlugins = getDefaultSerovalPlugins() | ||
| } | ||
|
|
||
| const contentType = request.headers.get('Content-Type') | ||
| const serovalPlugins = getDefaultSerovalPlugins() | ||
|
|
||
| function parsePayload(payload: any) { | ||
| const parsedPayload = fromJSON(payload, { plugins: serovalPlugins }) | ||
|
|
@@ -65,16 +68,16 @@ export const handleServerAction = async ({ | |
|
|
||
| const response = await (async () => { | ||
| try { | ||
| let result = await (async () => { | ||
| const result = await (async () => { | ||
| // FormData | ||
| if ( | ||
| formDataContentTypes.some( | ||
| FORM_DATA_CONTENT_TYPES.some( | ||
| (type) => contentType && contentType.includes(type), | ||
| ) | ||
| ) { | ||
| // We don't support GET requests with FormData payloads... that seems impossible | ||
| invariant( | ||
| method.toLowerCase() !== 'get', | ||
| methodLower !== 'get', | ||
| 'GET requests with FormData payloads are not supported', | ||
| ) | ||
| const formData = await request.formData() | ||
|
|
@@ -104,21 +107,19 @@ export const handleServerAction = async ({ | |
| } | ||
|
|
||
| // Get requests use the query string | ||
| if (method.toLowerCase() === 'get') { | ||
| invariant( | ||
| isCreateServerFn, | ||
| 'expected GET request to originate from createServerFn', | ||
| ) | ||
| // By default the payload is the search params | ||
| let payload: any = search.payload | ||
| if (methodLower === 'get') { | ||
| // Get payload directly from searchParams | ||
| const payloadParam = url.searchParams.get('payload') | ||
| // If there's a payload, we should try to parse it | ||
| payload = payload ? parsePayload(JSON.parse(payload)) : {} | ||
| const payload: any = payloadParam | ||
| ? parsePayload(JSON.parse(payloadParam)) | ||
| : {} | ||
| payload.context = { ...context, ...payload.context } | ||
| // Send it through! | ||
| return await action(payload, signal) | ||
| } | ||
|
Comment on lines
+110
to
120
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add error handling for JSON.parse to prevent unhandled exceptions. Line 115 calls 🔎 Proposed fix with error handling if (methodLower === 'get') {
// Get payload directly from searchParams
const payloadParam = url.searchParams.get('payload')
// If there's a payload, we should try to parse it
- const payload: any = payloadParam
- ? parsePayload(JSON.parse(payloadParam))
- : {}
+ let payload: any = {}
+ if (payloadParam) {
+ try {
+ payload = parsePayload(JSON.parse(payloadParam))
+ } catch (error) {
+ throw new Error('Invalid JSON payload in query parameter', { cause: error })
+ }
+ }
payload.context = { ...context, ...payload.context }
// Send it through!
return await action(payload, signal)
}🤖 Prompt for AI Agents |
||
|
|
||
| if (method.toLowerCase() !== 'post') { | ||
| if (methodLower !== 'post') { | ||
| throw new Error('expected POST method') | ||
| } | ||
|
|
||
|
|
@@ -127,18 +128,9 @@ export const handleServerAction = async ({ | |
| jsonPayload = await request.json() | ||
| } | ||
|
|
||
| // If this POST request was created by createServerFn, | ||
| // its payload will be the only argument | ||
| if (isCreateServerFn) { | ||
| const payload = jsonPayload ? parsePayload(jsonPayload) : {} | ||
| payload.context = { ...payload.context, ...context } | ||
| return await action(payload, signal) | ||
| } | ||
|
|
||
| // Otherwise, we'll spread the payload. Need to | ||
| // support `use server` functions that take multiple | ||
| // arguments. | ||
| return await action(...jsonPayload) | ||
| const payload = jsonPayload ? parsePayload(jsonPayload) : {} | ||
| payload.context = { ...payload.context, ...context } | ||
| return await action(payload, signal) | ||
| })() | ||
|
|
||
| // Any time we get a Response back, we should just | ||
|
|
@@ -148,37 +140,6 @@ export const handleServerAction = async ({ | |
| return result.result | ||
| } | ||
|
|
||
| // If this is a non createServerFn request, we need to | ||
| // pull out the result from the result object | ||
| if (!isCreateServerFn) { | ||
| result = result.result | ||
|
|
||
| // The result might again be a response, | ||
| // and if it is, return it. | ||
| if (result instanceof Response) { | ||
| return result | ||
| } | ||
| } | ||
|
|
||
| // TODO: RSCs Where are we getting this package? | ||
| // if (isValidElement(result)) { | ||
| // const { renderToPipeableStream } = await import( | ||
| // // @ts-expect-error | ||
| // 'react-server-dom/server' | ||
| // ) | ||
|
|
||
| // const pipeableStream = renderToPipeableStream(result) | ||
|
|
||
| // setHeaders(event, { | ||
| // 'Content-Type': 'text/x-component', | ||
| // } as any) | ||
|
|
||
| // sendStream(event, response) | ||
| // event._handled = true | ||
|
|
||
| // return new Response(null, { status: 200 }) | ||
| // } | ||
|
|
||
| if (isNotFound(result)) { | ||
| return isNotFoundResponse(result) | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.