Skip to content

fix: pass null instead of undefined to requestCompleted on success - #6837

Merged
Eomm merged 2 commits into
fastify:mainfrom
lazerg:fix/issue-6836-request-completed-null
Aug 6, 2026
Merged

fix: pass null instead of undefined to requestCompleted on success#6837
Eomm merged 2 commits into
fastify:mainfrom
lazerg:fix/issue-6836-request-completed-null

Conversation

@lazerg

@lazerg lazerg commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Node's finish event on http.ServerResponse fires with no arguments, so onResFinished in lib/reply.js was passing undefined to requestCompleted for successful responses instead of null. That didn't match the Error | null type in types/logger.d.ts (and the matching JSDoc in lib/log-controller.js), so code branching on error === null never matched the success case. Normalized to null at the call site and added a regression test.

Fixes #6836

@climba03003 climba03003 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather update the types instead of code.
Doing nullish assignment here for the undefined seems useless operation.

@github-actions github-actions Bot added the typescript TypeScript related label Jul 8, 2026
@lazerg

lazerg commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Pushed. Reverted the runtime change and widened requestCompleted's type to Error | null | undefined instead.

@lazerg
lazerg requested a review from climba03003 July 12, 2026 18:31
@lazerg

This comment was marked as abuse.

climba03003
climba03003 previously approved these changes Jul 15, 2026
@Tony133

Tony133 commented Aug 4, 2026

Copy link
Copy Markdown
Member

@lazerg As soon as you have time, can you resolve conflicts?

@lazerg
lazerg force-pushed the fix/issue-6836-request-completed-null branch from 0c3ff6d to 2454bf8 Compare August 4, 2026 09:18
@Tony133
Tony133 requested review from a team August 4, 2026 09:42
@Tony133
Tony133 requested a review from a team August 5, 2026 13:02
@Eomm
Eomm merged commit 9eaef51 into fastify:main Aug 6, 2026
36 checks passed
@Eomm Eomm added the bugfix Issue or PR that should land as semver patch label Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Issue or PR that should land as semver patch typescript TypeScript related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LogController.requestCompleted .d.ts error type inaccurate

5 participants