-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: allow to override serverfn method when using factories #5938
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
WalkthroughRemoves the per-method generic and Changes
Sequence Diagram(s)sequenceDiagram
participant Req as HTTP Request
participant Middleware as ServerFn Middleware
participant Handler as ServerFn Handler
Note over Req,Middleware: New flow — method sourced at runtime
Req->>Middleware: incoming request
Middleware->>Middleware: call getRequest() -> request.method
Middleware->>Handler: invoke with context + { method: request.method }
Handler->>Handler: reads context.method at runtime
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-10-08T08:11:47.088ZApplied to files:
🧬 Code graph analysis (2)e2e/solid-start/server-functions/src/routes/factory/index.tsx (1)
e2e/solid-start/server-functions/src/routes/factory/-functions/createFooServerFn.ts (1)
⏰ 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). (2)
🔇 Additional comments (5)
Comment |
|
View your CI Pipeline Execution ↗ for commit fcfc8c8
☁️ Nx Cloud last updated this comment at |
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: 0
🧹 Nitpick comments (1)
e2e/solid-start/basic-cloudflare/worker-configuration.d.ts (1)
16-17: ProcessEnv narrowing is fine; flag if you need widening later.Extending StringifyValues keeps
process.env.MY_VARtyped as the literal 'Hello from Cloudflare' (a subtype of string). If any consumer needs a plainstring, useconst v: string = process.env.MY_VARorString(process.env.MY_VAR).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
e2e/react-start/server-functions/src/routes/factory/-functions/createFooServerFn.ts(1 hunks)e2e/react-start/server-functions/src/routes/factory/index.tsx(7 hunks)e2e/solid-start/basic-cloudflare/worker-configuration.d.ts(1 hunks)packages/start-client-core/src/createServerFn.ts(2 hunks)packages/start-client-core/src/tests/createServerFn.test-d.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/start-client-core/src/tests/createServerFn.test-d.ts
🧰 Additional context used
🧬 Code graph analysis (2)
e2e/react-start/server-functions/src/routes/factory/index.tsx (1)
e2e/solid-start/server-functions/src/routes/factory/-functions/functions.ts (1)
composedFn(86-93)
e2e/react-start/server-functions/src/routes/factory/-functions/createFooServerFn.ts (2)
packages/start-server-core/src/request-response.ts (1)
getRequest(72-75)packages/start-client-core/src/createServerFn.ts (1)
createServerFn(49-168)
⏰ 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). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (7)
e2e/solid-start/basic-cloudflare/worker-configuration.d.ts (2)
5-7: LGTM: keep MY_VAR as a string literal type.Single-quoted literal is equivalent; type remains narrowed to 'Hello from Cloudflare'. No runtime impact.
11-14: Formatting-only change.Mapped type split over lines; no semantic changes. Good to keep.
e2e/react-start/server-functions/src/routes/factory/-functions/createFooServerFn.ts (2)
2-9: Middleware now correctly exposes HTTP method via contextImporting
getRequestand wiringrequest.methodintocontextin the server middleware is a good fit for the new API surface: handlers can now reliably read the actual HTTP verb viacontext.method, and the e2e factory tests will fail if the underlying request method doesn’t change as expected when overridingmethodon factories.Please ensure the React Start server-functions E2E suite (
/factoryroute) is run so we catch any regressions in environments wheregetRequest()might not be available.
17-18: Handler correctly switches frommethodparam tocontext.methodUpdating the handler to destructure only
{ context }and readcontext.methodaligns with the newServerFnCtxshape and the middleware-provided context, avoiding reliance on a top-levelctx.methodwhile still surfacing the HTTP verb to the user code.Double-check any other handlers in this E2E app that previously used a
methodparameter to ensure they are also migrated tocontext.methodwhere appropriate.e2e/react-start/server-functions/src/routes/factory/index.tsx (1)
40-122: Updated expectations correctly assertcontext.methodacross factory-based serverFnsThe expanded
expected.contextobjects for all serverFns (foo*, bar*, local*, composedFn) consistently include amethodfield that matches the intended HTTP verb ('GET'for default functions,'POST'for*POSTvariants) alongside the accumulated middleware context fields. This tightly couples the E2E surface to the new behavior: if method overriding via factories ever regresses, these tests will immediately fail via mismatchedcontext.method.Ensure the corresponding Solid Start factory tests stay in sync with this expectation pattern so both runtimes validate method propagation in the same way.
packages/start-client-core/src/createServerFn.ts (2)
161-165: Option merge infuncorrectly fixes method overriding for factoriesBuilding
newOptions = { ...resolvedOptions, ...options }and passing it as__optsintocreateServerFnensures that per-call overrides (especiallymethod) win over the factory’s base configuration, even when the factory itself was produced by earliermiddleware()/inputValidator()chaining. This directly addresses the “factories ignore{ method: 'POST' }at runtime” issue while preserving existing behavior for other options.Please run the factory-focused serverFn tests (including those creating nested factories) to confirm that
methodnow reflects the latest override in both the client fetcher and the server-side request.
315-322:ServerFnCtxnow reflects only data/context/signal; method moves into middleware-derived contextUpdating
ServerFnto acceptServerFnCtx<TRegister, TMiddlewares, TInputValidator>and redefiningServerFnCtxas{ data; context; signal }removesmethodfrom the top-level ctx type, pushing HTTP method awareness into the composedcontext(e.g., via middleware injectingcontext.method). This matches the E2E usage and keeps the core handler ctx minimal, whileServerFnMiddlewareOptions.methodstill carries the verb for middleware chains.Verify that any remaining handlers or type tests that previously accessed
ctx.methodhave been updated (or intentionally left) to usecontext.methodor middleware-level typing as appropriate.
b44f487 to
fcfc8c8
Compare
fixes: #5872
Summary by CodeRabbit
Refactor
Style
Tests
✏️ Tip: You can customize this high-level summary in your review settings.