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

Use chi.Router interface for AttachRoutes method #253

Closed
JVecsei1 opened this issue Jun 5, 2023 · 3 comments · Fixed by #254
Closed

Use chi.Router interface for AttachRoutes method #253

JVecsei1 opened this issue Jun 5, 2023 · 3 comments · Fixed by #254

Comments

@JVecsei1
Copy link

JVecsei1 commented Jun 5, 2023

Is your feature request related to a problem? Please describe.

For some methods chi returns the interface type chi.Router, e.g. when using the Route method on *chi.Mux to create a subrouter.

The AttachRoutes method currently uses the concrete implementation instead of the chi.Router interface, although it only relies on methods exposed by chi.Router.

Describe the solution you'd like

Changing the AttachRoutes method to accept the chi.Router interface type, instead of *chi.Mux would make it easier to use in some scenarios.

func AttachRoutes(router chi.Router, serviceBroker ServiceBroker, logger lager.Logger) {
	attachRoutes(router, serviceBroker, logger)
}

func attachRoutes(router chi.Router, serviceBroker ServiceBroker, logger lager.Logger) {
    // ...
}

Describe alternatives you've considered

Using a subrouter can still be achieved by creating a new router and mounting it manually, but methods such as Route or Group could only be used with type assertions.

subRouter := chi.NewRouter()
router.Mount("/my-route", subRouter)

Additional context

No response

@cf-gitbot
Copy link
Member

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/185332433

The labels on this github issue will be updated when the story is started.

blgm added a commit that referenced this issue Jun 6, 2023
The chi.Router interface should be accepted rather than the *chi.Mux
contete type. This allows for the contrete type to be substututed in
accordance with the [Liskov substitution
principle](https://en.wikipedia.org/wiki/Liskov_substitution_principle).

Resolves #253
@blgm
Copy link
Member

blgm commented Jun 6, 2023

Hi @JVecsei1. This makes complete sense. I've create a PR to resolve that. Before I merge it, I just thought I'd give you a chance to take a look and try it out, to make sure it's what you were expecting.

@JVecsei1
Copy link
Author

JVecsei1 commented Jun 6, 2023

Hi @JVecsei1. This makes complete sense. I've create a PR to resolve that. Before I merge it, I just thought I'd give you a chance to take a look and try it out, to make sure it's what you were expecting.

Hi @blgm ,

thanks a lot for the quick response & implementing the change. Looks great 👍

pivotal-marcela-campo pushed a commit that referenced this issue Jun 13, 2023
The chi.Router interface should be accepted rather than the *chi.Mux
contete type. This allows for the contrete type to be substututed in
accordance with the [Liskov substitution
principle](https://en.wikipedia.org/wiki/Liskov_substitution_principle).

Resolves #253
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants