-
Notifications
You must be signed in to change notification settings - Fork 117
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
implement new streamwriter api #291
base: main
Are you sure you want to change the base?
Conversation
5924d12
to
0cb498a
Compare
/// - length: Body size. Request validation will be failed with `HTTPClientErrors.contentLengthMissing` if nil, | ||
/// unless `Transfer-Encoding: chunked` header is set. | ||
/// - stream: Body chunk provider. | ||
public static func stream2(length: Int? = nil, _ stream: @escaping (StreamWriter2) -> Void) -> Body { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I we add method with the same name but different type we will break compilation, because some calls could become ambiguous 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that there is discussion about possibly breaking API in the nearish future anyway, we may find we want to delay this until then to resolve this problem.
0cb498a
to
30a7286
Compare
public struct StreamWriter2 { | ||
public let allocator: ByteBufferAllocator | ||
let onChunk: (IOData) -> EventLoopFuture<Void> | ||
let onComplete: EventLoopPromise<Void> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we're writing this as a callback-taking type rather than just holding onto the task object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and "forward" all write requests to it's channel? that might be a good idea, will try that, thanks!
/// - length: Body size. Request validation will be failed with `HTTPClientErrors.contentLengthMissing` if nil, | ||
/// unless `Transfer-Encoding: chunked` header is set. | ||
/// - stream: Body chunk provider. | ||
public static func stream2(length: Int? = nil, _ stream: @escaping (StreamWriter2) -> Void) -> Body { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that there is discussion about possibly breaking API in the nearish future anyway, we may find we want to delay this until then to resolve this problem.
Just as a heads up, the main development branch has been changed to This PR has been re-targeted to main and should just work. However when performing rebases etc please keep this in mind -- you may want to fetch the main branch and rebase onto the |
🚧 This is slightly WIP.
Revamps
StreamWriter
.Motivation:
There are two issues with current
StreamWriter
API:EventLoopFuture
is completed, which is not ideal.Modifications:
StreamWriter
APIStreamWriter2
(really need help with the naming here) with the following API:Result:
Closes #194
Closes #264