Skip to content

fix: don't use top level Response objects for controller response values #265 #268

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

Merged
merged 1 commit into from
Jun 20, 2025

Conversation

pablojhl
Copy link
Contributor

closes #265

@pablojhl pablojhl requested a review from tim-hm June 19, 2025 13:24
@pablojhl
Copy link
Contributor Author

pablojhl commented Jun 19, 2025

@tim-hm Is this what you had in mind? If yes, I'd update the other empty messages.

Copy link

github-actions bot commented Jun 19, 2025

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 85.21% 5077 / 5958
🔵 Statements 85.21% 5077 / 5958
🔵 Functions 77.31% 300 / 388
🔵 Branches 90.52% 659 / 728
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/builders/builders.controllers.ts 94.16% 100% 100% 94.16% 126-134
src/builders/builders.dto.ts 100% 100% 100% 100%
src/collections/collections.controllers.ts 90.51% 100% 100% 90.51% 236-247, 276-287
src/collections/collections.dto.ts 100% 100% 100% 100%
src/data/data.controllers.ts 100% 100% 100% 100%
src/data/data.dto.ts 100% 100% 100% 100%
src/queries/queries.controllers.ts 100% 100% 100% 100%
src/queries/queries.dto.ts 100% 100% 100% 100%
src/system/system.controllers.ts 96.21% 90% 100% 96.21% 122-129, 146-148
src/system/system.dto.ts 100% 100% 100% 100%
src/users/users.controllers.ts 92.67% 100% 100% 92.67% 72-81, 244-255
src/users/users.dto.ts 100% 100% 100% 100%
Generated in workflow #377 for commit 15e1fe7 by the Vitest Coverage Report Action

@tim-hm
Copy link
Collaborator

tim-hm commented Jun 19, 2025

@tim-hm Is this what you had in mind? If yes, I'd update the other empty messages.

I don't think we need to force the json response as that adds unnecessary overhead. I think we want to signal to callers that the response is 'empty' ,but as far as I can tell, hono always sends context-type: plain/text even if the response is new Response(null, { status: 201 }? Maybe we need to either explicitly sent the content-length or expliclity remove the header to get an empty body?

... what if we did something like just set the response to z.string() and then in the controller just used c.text("", StatusCode.CREATED)?

@pablojhl
Copy link
Contributor Author

@tim-hm Is this what you had in mind? If yes, I'd update the other empty messages.

I don't think we need to force the json response as that adds unnecessary overhead. I think we want to signal to callers that the response is 'empty' ,but as far as I can tell, hono always sends context-type: plain/text even if the response is new Response(null, { status: 201 }? Maybe we need to either explicitly sent the content-length or expliclity remove the header to get an empty body?

... what if we did something like just set the response to z.string() and then in the controller just used c.text("", StatusCode.CREATED)?

Sounds good. I will update the rest of empty messages.

@pablojhl pablojhl force-pushed the chore/empty-messages branch from 6fc51ea to a4e4313 Compare June 19, 2025 15:09
@pablojhl pablojhl marked this pull request as ready for review June 19, 2025 15:12
# Conflicts:
#	src/users/users.controllers.ts
@tim-hm tim-hm force-pushed the chore/empty-messages branch from a4e4313 to 15e1fe7 Compare June 20, 2025 10:57
@tim-hm tim-hm merged commit d850ddf into main Jun 20, 2025
2 checks passed
@tim-hm tim-hm deleted the chore/empty-messages branch June 20, 2025 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix: don't use top level Response objects for controller response values
2 participants