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

Remove NIOProtocolNegotiationResult #2554

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

FranzBusch
Copy link
Member

Motivation

After playing around more with the new async bootstrap methods, I came to the conclusion that the NIOProtocolNegotiationResult isn't carrying its weight. The NIOProtocolNegotiationResult is a glorified EventLoopFuture with an easy way to recursively resolve nested futures. Furthermore, it forces any nested protocol negotiation to use the same generic type for the end result.

Modification

This PR removes the NIOProtocolNegotiationResult and changes all the tests to be solely based on EventLoopFutures.

Result

Less code to support the async bootstrap and better composition of nested protocol negotiation handlers.

@Lukasa Lukasa added the 🆕 semver/minor Adds new public API. label Oct 16, 2023
# Motivation
After playing around more with the new async bootstrap methods, I came to the conclusion that the `NIOProtocolNegotiationResult` isn't carrying its weight. The `NIOProtocolNegotiationResult` is a glorified `EventLoopFuture` with an easy way to recursively resolve nested futures. Furthermore, it forces any nested protocol negotiation to use the same generic type for the end result.

# Modification
This PR removes the `NIOProtocolNegotiationResult` and changes all the tests to be solely based on `EventLoopFuture`s.

# Result
Less code to support the async bootstrap and better composition of nested protocol negotiation handlers.
@FranzBusch FranzBusch force-pushed the fb-remove-protocol-negotiation-result branch from 9575062 to c0abb3f Compare October 16, 2023 10:56
@FranzBusch FranzBusch enabled auto-merge (squash) October 16, 2023 10:56
@FranzBusch FranzBusch disabled auto-merge October 16, 2023 10:56
@FranzBusch
Copy link
Member Author

API breakage is expected. @Lukasa do you mind merging this

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants