br(v29): Making createServer() sync again#3472
Conversation
RobinTail
added
documentation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@pullfrog please review |
|
coverage: 100.0%. remained the same — sync-server-again into make-v29 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
express-zod-api/src/server.ts (1)
83-83: ⚡ Quick winFail fast if a lifecycle hook returns a Promise.
Line 83 and Line 93 now execute hooks synchronously, but JS callers (or
any-typed configs) can still return a
Promise, which is silently ignored. Add a runtime thenable guard so the new sync-only contract is enforced explicitly.Suggested patch
export const createServer = (config: ServerConfig, routing: Routing) => { + const ensureSyncHook = ( + name: "beforeRouting" | "afterRouting", + result: unknown, + ) => { + if ( + result && + typeof result === "object" && + "then" in result && + typeof (result as PromiseLike<unknown>).then === "function" + ) { + throw new Error(`${name} hook must be synchronous and return void.`); + } + }; + const { logger, getLogger, notFoundHandler, catcher, loggingMiddleware } = makeCommonEntities(config); @@ - config.beforeRouting?.({ app, getLogger }); + const beforeRoutingResult = config.beforeRouting?.({ app, getLogger }); + ensureSyncHook("beforeRouting", beforeRoutingResult); @@ - config.afterRouting?.({ app, getLogger }); + const afterRoutingResult = config.afterRouting?.({ app, getLogger }); + ensureSyncHook("afterRouting", afterRoutingResult);Based on learnings: PR objectives explicitly state
createServer()andbeforeRouting/afterRoutingare now synchronous.Also applies to: 93-93
🤖 Prompt for 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. In `@express-zod-api/src/server.ts` at line 83, The beforeRouting hook call at line 83 and the afterRouting hook call at line 93 currently execute synchronously but do not validate that callers don't return Promises, which would be silently ignored. Add a runtime thenable guard after each hook invocation to check if the return value is a Promise (by detecting if it has a .then method). If either hook returns a thenable, throw an error immediately to fail fast and enforce the explicit synchronous contract that the PR now requires for these lifecycle hooks.
🤖 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.
Nitpick comments:
In `@express-zod-api/src/server.ts`:
- Line 83: The beforeRouting hook call at line 83 and the afterRouting hook call
at line 93 currently execute synchronously but do not validate that callers
don't return Promises, which would be silently ignored. Add a runtime thenable
guard after each hook invocation to check if the return value is a Promise (by
detecting if it has a .then method). If either hook returns a thenable, throw an
error immediately to fail fast and enforce the explicit synchronous contract
that the PR now requires for these lifecycle hooks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0444f5f6-e1ea-4e81-8962-fdec1d78652c
📒 Files selected for processing (6)
README.mdexample/index.tsexpress-zod-api/src/config-type.tsexpress-zod-api/src/server.tsexpress-zod-api/tests/server.spec.tsexpress-zod-api/tests/system.spec.ts
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Reviewed changes — makes createServer() synchronous and removes Promise<void> from the ServerHook type, preventing async functions in beforeRouting/afterRouting hooks. This simplifies the API surface for beginners by eliminating the need for top-level await.
- Make
createServer()sync — removesasynckeyword fromcreateServerinsrc/server.tsand dropsawaitonbeforeRouting/afterRoutingcalls - Remove
Promise<void>fromServerHook— theServerHooktype insrc/config-type.tsis now() => voidonly, preventing async callbacks in hooks at compile-time - Update tests — all
async/awaitremoved fromcreateServercall sites intests/server.spec.ts;tests/system.spec.tsmoves the server-listening assertion into abeforeAllhook - Update example and docs —
example/index.tsandREADME.mddropawaitand explanatory comments
ℹ️ Migration rule and CHANGELOG not yet updated
This is a breaking change (removing async support from createServer and ServerHook). Per the project conventions in AGENTS.md, breaking changes to the public API require an update to the migration ESLint rule in migration/index.ts, its tests in migration/index.spec.ts, and CHANGELOG.md. Currently none of these have been updated.
Technical details
# Missing migration rule and CHANGELOG for sync createServer
## Affected sites
- `migration/index.ts` — no rule for this breaking change
- `migration/index.spec.ts` — no test coverage for the migration
- `CHANGELOG.md` — no entry for this change
## Required outcome
- A migration rule that removes `await` before `createServer(...)` calls
- A migration rule that removes `async` from functions passed to `beforeRouting`/`afterRouting` (and the `await` on their internal calls, if applicable)
- Valid/invalid test cases in `migration/index.spec.ts`
- A CHANGELOG entry for v9 documenting this change
## Open questions for the human
- Is this PR one of several incremental v9 changes, with migration/CHANGELOG to be done in a separate consolidation PR?DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — makes createServer() synchronous by removing Promise from ServerHook, simplifying the server bootstrap path now that async hooks are no longer required.
- Remove
Promise<void>fromServerHook—beforeRoutingandafterRoutinghooks are now sync-only, matching the new synccreateServer(). - Make
createServer()synchronous — dropsasync/awaitfrom the function and its hook invocations inserver.ts. - Update tests for sync
createServer— removesasync/awaitfrom test callbacks inserver.spec.ts; moves server-startup assertions intobeforeAllinsystem.spec.ts. - Update examples —
README.mdandexample/index.tsdropawaitfromcreateServer()calls.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — one new commit removes a redundant import { beforeAll } from "vitest" in the test file, since beforeAll is a Vitest global.
- Remove redundant
beforeAllimport —express-zod-api/tests/system.spec.tshad an explicit import for a Vitest global; cleaned up.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
RobinTail
changed the title
createServer() sync againcreateServer() sync again
1 participant
Thanks to #3465
This should simplify daily routines for beginners.
Tradeoffs
PromisefromServerHooktype =>beforeRoutingandafterRoutingbecome synccreateServerSummary by CodeRabbit
Documentation
awaitRefactor
awaitwhen invoking it