feat(server): New Tracer API #2068
Conversation
WalkthroughThe changes update the repository’s ignore rules and integrate tracing into the server adapter. Three new entries were added to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ServerAdapter
participant Plugin
participant Tracer
participant RequestHandler
Client->>ServerAdapter: Receives request
ServerAdapter->>Plugin: Check for tracer
Plugin-->>ServerAdapter: Provides tracer (if available)
alt Tracer exists
ServerAdapter->>Tracer: Log request & wrap handler
Tracer-->>ServerAdapter: Return wrapped handler
end
ServerAdapter->>RequestHandler: Process request
RequestHandler-->>ServerAdapter: Return response
ServerAdapter-->>Client: Send response
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
✅
|
✅
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/server/src/createServerAdapter.ts (3)
104-104: Optional doc comment fortracer
Consider adding a brief explanation of thetracerproperty to clarify its optional usage and purpose.
148-155: Improve error details when multiple tracers are found
This block properly enforces the single-tracer constraint, but it might be helpful to include the plugin's identifier or name in the error message to aid in debugging.- throw new Error( - 'Multiple tracers have been declared. Only one plugin with a tracer is allowed.', - ); + throw new Error( + `Multiple tracers have been declared. Only one plugin with a tracer is allowed. Plugin: ${plugin.name || 'unknown'}` + );
252-262: Handle potential undefinedresponse
Iftracer.requestnever invokes the wrapped function,responsemay remain undefined. Consider providing a fallback response or throwing an error, rather than relying on the non-null assertion.- return isPromise(tracerPromise) ? tracerPromise.then(() => response) : response!; + return isPromise(tracerPromise) + ? tracerPromise.then(() => response ?? new Response('No response returned by tracer', { status: 500 })) + : (response ?? new Response('No response returned by tracer', { status: 500 }));packages/server/src/plugins/types.ts (2)
1-1: Remove unused import or adjust usage
The importedMaybePromiseis currently unused. If you plan to unify your promise signatures withMaybePromise, consider applying it to your tracer definition. Otherwise, remove the import.
53-58: Unify signature withMaybePromise
You already importMaybePromise; consider using it here for consistency, for instance:
request: (payload: { request: Request }, wrapped: () => MaybePromise<void>) => MaybePromise<void>;-export type Tracer = { - request: ( - payload: { request: Request }, - wrapped: () => Promise<void> | void, - ) => Promise<void> | void; -}; +export type Tracer = { + request: ( + payload: { request: Request }, + wrapped: () => MaybePromise<void>, + ) => MaybePromise<void>; +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)packages/server/src/createServerAdapter.ts(5 hunks)packages/server/src/plugins/types.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🪛 Biome (1.9.4)
packages/server/src/plugins/types.ts
[error] 8-8: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / deno
- GitHub Check: unit / node 20
- GitHub Check: unit / bun
- GitHub Check: esm
- GitHub Check: server (undici)
- GitHub Check: e2e / cloudflare-modules
- GitHub Check: server (ponyfill)
- GitHub Check: e2e / cloudflare-workers
- GitHub Check: unit / node 18
- GitHub Check: lint
- GitHub Check: e2e / azure-function
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: prettier
- GitHub Check: server (native)
- GitHub Check: e2e / aws-lambda
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: type check
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
packages/server/src/createServerAdapter.ts (2)
3-3: Looking good for the new import ofTracer
No issues spotted with the added import statement.
176-176: Allowing re-assignment of handleRequest
Switching fromconsttoletis correct to accommodate wrapping the request handler with tracer logic.packages/server/src/plugins/types.ts (1)
9-13: Typo in documentation and consider making property optional
- "insance" should be "instance".
- The usage in
createServerAdapterchecks ifplugin.tracerexists, so consider markingtraceras optional (tracer?: Tracer;).
[nitpick, refactor_suggestion_good_to_have]- * A tracer insance. It can be used to wrap... + * A tracer instance. It can be used to wrap...
✅
|
✅
|
✅
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/server/src/plugins/types.ts (2)
8-12: Fix typo in JSDoc comment.There's a typo in the JSDoc comment: "insance" should be "instance".
- * A tracer insance. It can be used to wrap the entire request handling pipeline (including the + * A tracer instance. It can be used to wrap the entire request handling pipeline (including the
52-57: Add JSDoc documentation and consider error handling.The
Tracertype would benefit from:
- JSDoc documentation explaining its purpose and usage
- Explicit error handling guidance for the wrapped function
Add JSDoc documentation and consider error handling:
export type Tracer = { + /** + * Wraps a request handling operation for tracing purposes. + * @param payload - The payload containing the request to be traced + * @param wrapped - The function to be traced + * @throws - Document expected errors and how they should be handled + * @returns A promise that resolves when the traced operation completes + */ request: ( payload: { request: Request }, wrapped: () => Promise<void> | void, ) => Promise<void> | void; };Consider adding error handling guidance by documenting:
- What happens if the wrapped function throws?
- Should the tracer catch and log errors?
- Should errors be propagated or transformed?
Would you like me to propose a more detailed error handling implementation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/server/src/plugins/types.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: unit / node 22
- GitHub Check: unit / node 18
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@whatwg-node/server |
0.10.0-alpha-20250218194158-de7e6d1454b125b67766395ce9bc8509d8900f93 |
npm ↗︎ unpkg ↗︎ |
@whatwg-node/server-plugin-cookies |
2.0.0-alpha-20250218194158-de7e6d1454b125b67766395ce9bc8509d8900f93 |
npm ↗︎ unpkg ↗︎ |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/purple-buses-try.md (1)
5-6: Expand on the Tracer API Description for Better Developer Guidance
The short description "Add new Tracer API" is clear but could benefit from additional details. Consider adding more context about the Tracer API's role (e.g., its purpose for wrapping request phases or performance monitoring) or linking to further documentation. This would help developers understand the impact and usage of the new feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.changeset/purple-buses-try.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
- GitHub Check: type check
- GitHub Check: server (undici)
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: unit / deno
- GitHub Check: server (ponyfill)
- GitHub Check: unit / bun
- GitHub Check: e2e / azure-function
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: server (native)
🔇 Additional comments (1)
.changeset/purple-buses-try.md (1)
1-3: Ensure Consistent Version Bumping for the New Tracer API
The changeset correctly indicates a minor version update for the package@whatwg-node/server, which aligns with adding a new feature. Ensure that this update is consistent with the release strategy and that related documentation (e.g., changelogs and release notes) is updated accordingly.
| @@ -78,3 +78,6 @@ package-lock.json | |||
| eslint_report.json | |||
|
|
|||
| deno.lock | |||
| .helix/config.toml | |||
There was a problem hiding this comment.
That's because I'm trying to use another editor than VScode ^^'
It's Helix's configuration file :-)
Actually I can commit it if someone else also want to use this editor :-)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/server/src/createServerAdapter.ts (1)
176-176: Consider simplifying the tracer implementation.The tracing implementation is solid but could be more concise.
Consider this simplified version:
-if (tracer?.request) { +if (tracer) { const originalRequestHandler = handleRequest; handleRequest = (request, initialContext) => { let response: Promise<Response> | Response; - const tracerPromise = tracer!.request!({ request }, () => { + const tracerPromise = tracer.request?.({ request }, () => { response = originalRequestHandler(request, initialContext); return isPromise(response) ? response.then(() => undefined) : undefined; }); - return isPromise(tracerPromise) ? tracerPromise.then(() => response) : response!; + return tracerPromise ? tracerPromise.then(() => response) : response; }; }This version:
- Removes unnecessary non-null assertions
- Uses optional chaining more effectively
- Simplifies the return statement
Also applies to: 252-262
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/server/src/createServerAdapter.ts(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: unit / node 23
- GitHub Check: unit / deno
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: unit / bun
- GitHub Check: server (undici)
- GitHub Check: unit / node 18
- GitHub Check: server (ponyfill)
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: server (native)
- GitHub Check: e2e / azure-function
- GitHub Check: node-fetch (noConsumeBody)
🔇 Additional comments (2)
packages/server/src/createServerAdapter.ts (2)
3-3: LGTM!Clean import of the
Tracertype, maintaining consistency with existing imports.
104-104: LGTM! Robust tracer validation.The implementation ensures that only one tracer can be declared, with proper error handling and clear error messages.
Also applies to: 148-155
Description
This PR is the continuation of n1ru4l/envelop#2430 which introduces a new core API: The Tracer.
A plugin (and only one) can define a
tracerinstance that will wrap every phases of a request pipeline.In the case of
wahtwg-node, it only adds therequestwrapper.Type of change
Please delete options that are not relevant.
Summary by CodeRabbit
Chores
New Features