Skip to content
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

2.x.x - Trailer headers #330

Merged
merged 6 commits into from
Jan 11, 2024
Merged

2.x.x - Trailer headers #330

merged 6 commits into from
Jan 11, 2024

Conversation

adam-fowler
Copy link
Member

Trailer headers are output as the return value from the response body writer

Copy link

codecov bot commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1e3b93d) 83.08% compared to head (39d0843) 83.12%.

Additional details and impacted files
@@            Coverage Diff             @@
##            2.x.x     #330      +/-   ##
==========================================
+ Coverage   83.08%   83.12%   +0.03%     
==========================================
  Files          92       92              
  Lines        5037     5048      +11     
==========================================
+ Hits         4185     4196      +11     
  Misses        852      852              

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

Copy link

github-actions bot commented Jan 5, 2024

@Joannis
Copy link
Contributor

Joannis commented Jan 6, 2024

Many users won't use the trailing headers, does it make sense to support an additional closure that's (still) defines as -> Void for users that don't need trailing headers? We could then make the trailing headers return a non-optional, so it's clearly distinct.

@adam-fowler
Copy link
Member Author

Many users won't use the trailing headers, does it make sense to support an additional closure that's (still) defines as -> Void for users that don't need trailing headers? We could then make the trailing headers return a non-optional, so it's clearly distinct.

I tried having two different init one with a handler that returned Void and one with a handler that return HTTPFields but it confuses the compiler. If you have a closure where it isn't obvious what the return value you get an error. I think I have a solution though checkout the last commit.

/// written
public static func withTrailingHeaders(
contentLength: Int? = nil,
_ write: @Sendable @escaping (any HBResponseBodyWriter) async throws -> HTTPFields?
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this a lot better, now one more idea to play with: Can we make the any HBResponseBodyWriter an inout? This would make it harder to incorrectly wield this API.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what you are getting at there. What do we gain from making any HBResponseBodyWriter inout. That closure is called by HTTPChannelHandler and any Middleware that want to process response bodies. ie very few people will be calling it.

Are you seeing a situation where you would want to the write function to mutate the HBResponseBodyWriter?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I don't see a situation where we'd want to mutate. I see a risk in people trying to escape that HBResponseBodyWriter out of this closure

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want to hold up this PR for this topic though, since it's not caused by this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

ok I see, by making it inout you make it harder but it could still be done. I could make ~Copyable I don't know if that'd help

Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot that HBResponseBodyWriter is a protocol not a struct

Copy link
Member Author

@adam-fowler adam-fowler Jan 11, 2024

Choose a reason for hiding this comment

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

Adding an issue to chase this up #342

@adam-fowler adam-fowler changed the title Trailer headers 2.x.x - Trailer headers Jan 8, 2024
@adam-fowler adam-fowler merged commit dde5dfa into 2.x.x Jan 11, 2024
4 of 5 checks passed
@adam-fowler adam-fowler deleted the trailer-headers branch January 11, 2024 07:07
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.

None yet

2 participants