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

Remove NamedService bound for tonic_web::enable #1115

Closed
slinkydeveloper opened this issue Oct 18, 2022 · 3 comments · Fixed by #1119
Closed

Remove NamedService bound for tonic_web::enable #1115

slinkydeveloper opened this issue Oct 18, 2022 · 3 comments · Fixed by #1119

Comments

@slinkydeveloper
Copy link
Contributor

Bug Report

tonic_web::enable has a bound on NamedService used only for logging purposes.

Version

➜ cargo tree | grep tonic
│   ├── tonic v0.8.0
│   ├── tonic v0.8.0 (*)
├── tonic v0.8.0 (*)
├── tonic-web v0.4.0
│   ├── tonic v0.8.0 (*)
└── tonic-build v0.8.0
├── tonic v0.8.0 (*)

Crates

  • tonic-web

Description

This bound is not really needed, and requires to be implemented in cases such as when users manually implement the underlying service. Does it makes sense to remove it?
@LucioFranco
Copy link
Member

Yeah, seems fair to remove, I believe we could just pull the path for logging to check the service.

@slinkydeveloper
Copy link
Contributor Author

WDYM by pulling the path? Removing it altogether?

slinkydeveloper added a commit to slinkydeveloper/tonic that referenced this issue Oct 19, 2022
@slinkydeveloper
Remove NamedService bound. Fix hyperium#1115
24d37be 
Signed-off-by: slinkydeveloper <[email protected]>
@slinkydeveloper slinkydeveloper mentioned this issue Oct 19, 2022
Merged
@LucioFranco
Copy link
Member

The path from the http Request type that comes in, it should contain something like/package/ServceName

slinkydeveloper added a commit to slinkydeveloper/tonic that referenced this issue Oct 24, 2022
@slinkydeveloper
Remove NamedService bound. Fix hyperium#1115
867b6d3 
Signed-off-by: slinkydeveloper <[email protected]>
@softdevca softdevca mentioned this issue Dec 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Implement tower::Layer for tonic_web::Config slinkydeveloper/tonic
2 participants
@LucioFranco @slinkydeveloper