-
-
Notifications
You must be signed in to change notification settings - Fork 448
types: Mark app.server as non-null in START event handler #1651
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughReplaces the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
88-167: AddServerStartHandlerto root exports and fix JSDoc template literalThe signature change for
onStart(handler: MaybeArray<ServerStartHandler<this>>)correctly aligns with the updatedLifeCycleStore.starttyping. However, two issues prevent this from being a complete public API addition:
ServerStartHandleris not re-exported from the root moduleThe type is imported from
./typesbut missing from the mainexport type { … }block. Users cannot currently import it viaimport type { ServerStartHandler } from 'elysia'. Add it alongside other lifecycle types likeGracefulHandler.
- JSDoc example uses double quotes instead of a template literal
The example string
"Running at ${server.url}:${server.port}"won't interpolate. Change to backticks:- * console.log("Running at ${server.url}:${server.port}") + * console.log(`Running at ${server.url}:${server.port}`)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/index.tssrc/types.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/types.ts (1)
src/index.ts (1)
AnyElysia(175-175)
src/index.ts (1)
src/types.ts (2)
MaybeArray(284-284)ServerStartHandler(1461-1463)
🔇 Additional comments (1)
src/types.ts (1)
626-639: ServerStartHandler typing forstarthooks looks sound; just confirm adapter invariantsTyping
LifeCycleStore.startasHookContainer<ServerStartHandler<any>>[]and definingServerStartHandleras receivingInstance & { server: NonNullable<Instance["server"]> }matches the intended contract thatserveris definitely set by the timestarthooks run. This should give nice, precise types for both.onStartand.on('start', …).The only thing to double‑check is that every
ElysiaAdapter.listenimplementation (Bun, Node, Web Standard, etc.) continues to setapp.serverbefore invoking anystarthandlers, and never invokes them in contexts whereservermight still benull. If that invariant ever changes, this new type will become unsound even though the runtime behavior is unchanged.Also applies to: 1461-1463
What?
This PR changes the typing of the handlers for the
startevent, declaringapp.serveras non-null for them.It also updates the example snipped on
onStart, removing the optional-chaining that is not needed because of this change anymore.Why?
A miniscule, personal, stylistic annoyance:
After this change, this will work just fine:
(The optional-chaining ("
null-check") is no longer needed)I guess another option would be using
app.listen's callback, but that's only feasible foronStartevent handlers on the "main"Elysiainstance (i.e. outside of plugins).As far as I was able to tell the
"start"event handlers are never called from outside oflisten, whereapp.serveris set withBun.serve(…)just before. SinceBun.serve(…)does not returnnull(orundefined) (according to TypeScript, and the docs don't mention anything contrary), this should be a safe guarantee to make. For what it's worth,tsc(bun run test:types) doesn't have any issues with the changes either.I completely understand if this can't/won't be merged, be it because it's intentionally typed the current way for future changes/expansions or just because it's super insignificant; as mentioned, it was just a super small annoyance to me and I figured it'd be easy (and quick) enough to change.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.