-
-
Notifications
You must be signed in to change notification settings - Fork 88
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
fix(server): pass custom logger to api handler #1783
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to three handler functions across different packages: Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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
🧹 Outside diff range and nitpick comments (2)
packages/server/src/hono/handler.ts (2)
Line range hint
11-16
: Add thelogger
property to theHonoOptions
interfaceThe
HonoOptions
interface is missing thelogger
property that was mentioned in the PR summary. This property is being used in thecreateHonoHandler
function, so it should be defined in the interface for type safety.Please update the
HonoOptions
interface to include thelogger
property:export interface HonoOptions extends AdapterBaseOptions { /** * Callback method for getting a Prisma instance for the given request. */ getPrisma: (ctx: Context) => Promise<unknown> | unknown; + logger: unknown; }
55-55
: LGTM! Consider adding a comment for clarityThe addition of the
logger
property to therequestHandler
function call is correct and aligns with the PR objective. This change enhances the logging capabilities during request processing.For improved clarity, consider adding a brief comment explaining the purpose of the
logger
property:requestHandler({ method: ctx.req.method, path, query, requestBody, prisma, modelMeta, zodSchemas, + // Pass custom logger to enhance request processing visibility logger: options.logger, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/server/src/hono/handler.ts (1 hunks)
- packages/server/src/nuxt/handler.ts (1 hunks)
- packages/server/src/sveltekit/handler.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
packages/server/src/nuxt/handler.ts (2)
Line range hint
53-63
: Consider enhancing error loggingWhile the
logger
is now passed to therequestHandler
, there are two areas where we could potentially improve its usage:
Verify that the
requestHandler
implementation correctly utilizes the newlogger
property. This is crucial for ensuring that the logging enhancement is effective.Consider using the
logger
in the error handling block (lines 59-62). Currently, errors are only returned in the response body. Logging these errors could provide valuable insights for debugging and monitoring. For example:} catch (err) { setResponseStatus(event, 500); options.logger?.error(`Unhandled error in request handler: ${err}`); return { message: `An unhandled error occurred: ${err}` }; }To verify the
requestHandler
implementation, please run the following script:#!/bin/bash # Description: Verify the requestHandler implementation # Test: Check if requestHandler uses the logger property ast-grep --lang typescript --pattern 'function $_($_{ $$$ logger: $_ $$$ }) { $$$ logger.$_($_) $$$ }'
53-53
: LGTM. Consider backwards compatibility and documentation.The addition of the
logger
option aligns well with the PR objective of passing a custom logger to the API handler. This enhancement will improve logging capabilities during request processing.A few points to consider:
Ensure that the
HandlerOptions
interface has been updated to include thelogger
property. This update is mentioned in the AI summary but isn't visible in the provided code.Consider backwards compatibility. If
logger
is not optional inHandlerOptions
, this change could potentially break existing code that doesn't provide a logger. Consider making it optional (logger?: unknown
) if it hasn't been done already.It would be helpful to add documentation for the new
logger
option, explaining its purpose and expected type.To verify the
HandlerOptions
interface update, please run the following script:packages/server/src/sveltekit/handler.ts (2)
Line range hint
1-89
: Verify consistency of logger implementation across the codebaseThe addition of the
logger
property to therequestHandler
function call is a good improvement. To ensure consistency, we should verify that this change has been applied to all relevant parts of the codebase.Please run the following verification:
#!/bin/bash # Description: Verify consistent implementation of logger across the codebase # Test 1: Search for other occurrences of requestHandler to ensure logger is passed consistently rg --type typescript 'requestHandler\s*\(' -A 5 -g '!**/handler.ts' # Test 2: Search for HandlerOptions interface to ensure logger property is added rg --type typescript 'interface\s+HandlerOptions' # Expected results: # Test 1: All occurrences of requestHandler should include the logger property # Test 2: HandlerOptions interface should include a logger property
65-65
: LGTM: Logger successfully passed to requestHandlerThe addition of the
logger
property to therequestHandler
function call is consistent with the PR objective and enhances the logging capabilities of the request handling process.To ensure the
requestHandler
function is prepared to handle the newlogger
property, please run the following verification:
No description provided.