Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(context): single body overrides other returns #3800

Merged

Conversation

askorupskyy
Copy link
Contributor

The author should do the following, if applicable

  • Add tests
  • Run tests
  • bun run format:fix && bun run lint:fix to format the code
  • Add TSDoc/JSDoc to document the code

Context

Resolves #3798

@askorupskyy
Copy link
Contributor Author

@lewisedc, @cybercoder-naj pls review this PR to make sure it resolves your issue

expect(res.headers.get('X-Foo')).toBe(null)
c.header('X-Foo2', undefined)
res = c.res
expect(res.headers.get('X-Foo2')).toBe(null)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

c.body() now has a type of Response & TypedResponse, so this change was made. The types are no longer compatible

Copy link

codecov bot commented Jan 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.71%. Comparing base (fa6f51b) to head (92a1a12).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #3800    +/-   ##
========================================
  Coverage   91.70%   91.71%            
========================================
  Files         160      160            
  Lines       10192    10195     +3     
  Branches     2997     2879   -118     
========================================
+ Hits         9347     9350     +3     
  Misses        844      844            
  Partials        1        1            

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

(data: Data, init?: ResponseOrInit<ContentfulStatusCode>): Response
(data: null, init?: ResponseOrInit): Response
<U extends ContentfulStatusCode>(data: Data, status?: U, headers?: HeaderRecord): Response &
TypedResponse<unknown, U, 'body'>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yusukebe not sure if this should remain as unknown, could you advise pls?

Copy link
Member

Choose a reason for hiding this comment

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

@askorupskyy

unknown is OK. _data in TypedResponse should be unknown in this case.

@lewisedc
Copy link

lewisedc commented Jan 4, 2025

Just tested this and it's working for me! Thanks @askorupskyy

Copy link

@cybercoder-naj cybercoder-naj left a comment

Choose a reason for hiding this comment

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

Lgtm thanks!

(data: Data, init?: ResponseOrInit<ContentfulStatusCode>): Response
(data: null, init?: ResponseOrInit): Response
<U extends ContentfulStatusCode>(data: Data, status?: U, headers?: HeaderRecord): Response &
TypedResponse<unknown, U, 'body'>
Copy link
Member

Choose a reason for hiding this comment

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

@askorupskyy

unknown is OK. _data in TypedResponse should be unknown in this case.

})

type Actual = ExtractSchema<typeof router>['/']['$get']['status']
expectTypeOf<Actual>().toEqualTypeOf<204 | 201 | 200>()
Copy link
Member

Choose a reason for hiding this comment

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

This test is very good!

Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

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

LGTM!

@yusukebe
Copy link
Member

yusukebe commented Jan 5, 2025

@askorupskyy Nice! Thank you very much for handling it.

@cybercoder-naj @lewisedc Thank you for the issue and the review!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A single c.body() return in a handler overrides all other returns
4 participants