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.

Sign up for GitHub

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

Jump to bottom

feat(client): change Connect trait into an alias for Service #1912

Merged
merged 1 commit into from
Oct 22, 2019
Merged

feat(client): change Connect trait into an alias for Service #1912

merged 1 commit into from
Oct 22, 2019

Conversation

seanmonstar
Copy link
Member

The Connect trait is now essentially an alias for
Service<Destination>, with a blanket implementation as such, and is
sealed.

Closes #1902

BREAKING CHANGE: Any manual implementations of Connect must instead
implement tower::Service<Destination>.

@DoumanAsh
Copy link
Contributor

DoumanAsh commented Aug 22, 2019 •
edited
Loading

Can you then re-export tower::Service in hyper?

@seanmonstar
Copy link
Member Author

Can you then re-export tower::Service in hyper?

Yes, I think it should be. On master right now, there is hyper::service::Service, which is basically an alias for tower::Service<http::Request<B>> (so sealed and a blanket impl). Perhaps that should be renamed (hyper::service::alias::Service or hyper::service::HttpService or something).

@DoumanAsh
Copy link
Contributor

Yeah, my library implements own connector now so ideally I'd prefer to depend on hyper traits, without need to keep tower-service up to date.
Also would be nice to have aliases for original traits like Connect that user need to implement rather than sealed so that we could just implement straight ahead it

LucioFranco
LucioFranco requested changes Aug 27, 2019
View reviewed changes
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

So I'm not sure if this is the right route just yet. I originally wanted to introduce MakeConnection into this mix slowly. I was only going to introduce MakeConnection within client Connect (new make service for SendRequest).

That said, why not use Connect as a local version of HttpMakeConnection? So that means instead to take a &mut self and follow towers pattern? I will draft my PR here in a few to see which path we prefer.

@LucioFranco
Copy link
Member

The other thing is I want to move away from the (TcpStream, Connected) pattern and start using HttpConnection as this simplifies things imo.

@LucioFranco
Copy link
Member

Ok, I opened a draft Pr with what I was working on over the weekend #1915

@seanmonstar seanmonstar force-pushed the connect-as-service branch 5 times, most recently from 9f2fda8 to 846b31b Compare October 22, 2019 20:42
@seanmonstar
feat(client): change Connect trait into an alias for Service
64318cc 
The `Connect` trait is now essentially an alias for
`Service<Destination>`, with a blanket implementation as such, and is
sealed.

Closes #1902

BREAKING CHANGE: Any manual implementations of `Connect` must instead
  implement `tower::Service<Destination>`.
@seanmonstar seanmonstar merged commit d67e49f into master Oct 22, 2019
@seanmonstar seanmonstar deleted the connect-as-service branch October 22, 2019 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LucioFranco LucioFranco LucioFranco approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change Connect trait to alias Service
3 participants
@seanmonstar @DoumanAsh @LucioFranco