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

Remove dependency on github.com/go-chi/chi/v5 (again) #322

Closed
rhcarvalho opened this issue May 15, 2024 · 4 comments
Closed

Remove dependency on github.com/go-chi/chi/v5 (again) #322

rhcarvalho opened this issue May 15, 2024 · 4 comments
Labels
go1.22 changes that requires Go 1.22

Comments

@rhcarvalho
Copy link

Hi! I'm evaluating the dependency tree of a project that uses github.com/mailgun/mailgun-go/v4. The project doesn't use the MockServer, still github.com/go-chi/chi/v5 ends up in the dependency tree via mailgun-go.

I see this library went from chi to gorilla/mux (#252) and back to chi (#307) in the last few years.

Now with Go 1.22 "enhanced routing patterns" there's a good chance to drop external dependencies in favor of using the standard library. For the usage I saw in those PRs the stdlib would be a good fit.

I see go.mod is currently at 1.13 which was before the go directive started to mean minimal Go toolchain version. And I didn't find any mention in the README with regards to supported Go versions. Go itself supports the last two major releases.

Would a PR to remove the external dependency be accepted? If so, should it wait until 1.23 is released?

@vtopc
Copy link
Contributor

vtopc commented Sep 13, 2024

Hi, @rhcarvalho

  1. What's wrong with github.com/go-chi/chi or gorilla/mux? They are pretty lightweight libraries without any external dependencies.
  2. The problem with the new https://go.dev/doc/go1.22#enhanced_routing_patterns is that they put both: the optional method and path into one string, so it's pretty easy to do a typo error there, e.g.:
  • chi's r.Got("/foo", handler) - is a compile error;
  • net/http's r.Handle("GOT /foo", handler) - is a runtime error, it would be detected only while running the code or in unit tests(assuming there is a 100% code coverage). Of course r.Handle(http.MethodGet + " /foo", handler) is an option, but now space makes sense and might be missed.

@vtopc vtopc added the go1.22 changes that requires Go 1.22 label Sep 13, 2024
@rhcarvalho
Copy link
Author

Hi @vtopc, thanks for the considerations.

  1. Nothing wrong with those projects, they have their undeniable merits.

    My suggestion was based on the fact that "this lib is only relying on chi for testing and mocking" (quoting Swap chi router for gorilla mux #252 (comment)) and when inspecting a larger dependency tree of a project it was unexpected that the Mailgun client, which should instinctively be an HTTP client, depends on a HTTP server routing library.

  2. Good point. Concerns in that direction (e.g. net/http: enhanced ServeMux routing golang/go#61410 (comment)) have been raised upstream but the implementation nonetheless favored the way it was implemented, having to balance different tradeoffs.

    I can see how the impact on developer experience can be decisive in the context of a project whose main concern is being a web server, but it is less clear to me how it would affect testing the Mailgun client.

All that said, I don't feel strong about it and wouldn't mind if we just close this issue and move on :)

@rhcarvalho
Copy link
Author

BTW nice to see you're picking up maintenance of the library 🚀

@vtopc
Copy link
Contributor

vtopc commented Sep 16, 2024

it was unexpected that the Mailgun client, which should instinctively be an HTTP client, depends on a HTTP server routing library

Initially, mocks were used for internal testing, but now things have gone too far, as I see that some consumers are using them for their tests too. We could get rid of that in v5.

All that said, I don't feel strong about it and wouldn't mind if we just close this issue and move on :)

OK. Closing.

@vtopc vtopc closed this as completed Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go1.22 changes that requires Go 1.22
Projects
None yet
Development

No branches or pull requests

2 participants