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

feat!: remove dependency on go-chi/chi #340

Merged
merged 2 commits into from
Dec 20, 2024
Merged

feat!: remove dependency on go-chi/chi #340

merged 2 commits into from
Dec 20, 2024

Conversation

blgm
Copy link
Member

@blgm blgm commented Dec 5, 2024

BREAKING CHANGE

In Go 1.22, http.ServeMux was improved so that we no longer need to use
go-chi/chi, gorilla/mux, or any other thrid party HTTP router. The advantage to consumers
is that they can use any router, or none, and do not have a dependency forced on them.

Public API changes:

  • New() has changed signature to optionally take options.

  • AttachRoutes() has been removed. If you want to attach to an existing Chi router, you can still
    do something like:

b := brokerapi.New(broker, logger, brokerapi.WithBrokerCredentials(creds))

r := chi.NewRouter()
r.Handle("/*", b)
  • The WithRouter() option has been removed as a Chi router can no longer be specified.

  • WithEncodedPath() has been removed as it was deprecated and did nothing.

go.mod Outdated Show resolved Hide resolved
api.go Show resolved Hide resolved
api.go Outdated Show resolved Hide resolved
@blgm blgm force-pushed the chiless branch 3 times, most recently from 8413428 to 519c799 Compare December 11, 2024 13:59
BREAKING CHANGE

In Go 1.22, http.ServeMux was improved so that we no longer need to use
go-chi/chi, gorilla/mux, or any other thrid party HTTP router. The advantage to consumers
is that they can use any router, or none, and do not have a dependency forced on them.

Public API changes:

- `New()` has changed signature to optionally take options.

- `AttachRoutes()` has been removed. If you want to attach to an existing Chi router, you can still
do something like:
```go
b := brokerapi.New(broker, logger, brokerapi.WithBrokerCredentials(creds))

r := chi.NewRouter()
r.Handle("/*", b)
```

- The `WithRouter()` option has been removed as a Chi router can no longer be specified.

- `WithEncodedPath()` has been removed as it was deprecated and did nothing.

fakeResponseWriter = new(fakes.FakeResponseWriter)
fakeResponseWriter.HeaderReturns(http.Header{})
fakeServer = httptest.NewServer(brokerapi.NewWithOptions(fakeServiceBroker, slog.New(slog.NewJSONHandler(GinkgoWriter, nil))))
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: nice to see the Standard library solution for HTTP tests

var cfg config
WithOptions(opts...)(&cfg)

mw := append(append(cfg.authMiddleware, defaultMiddleware(logger)...), cfg.additionalMiddleware...)
Copy link
Contributor

Choose a reason for hiding this comment

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

question: I think we have to change this order, what do you think?

mw := append(append(cfg.additionalMiddleware, defaultMiddleware(logger)...), cfg.authMiddleware...)

@zucchinidev
Copy link
Contributor

One more question before merging, @blgm

I hope I am not confusing you.
The first executed middleware is the last in the array cfg.additionalMiddleware. This is because the middlewares are reversed before being applied, so the last middleware in the array becomes the first to execute. So, do we want the auth middleware to be the last in the array?

@blgm
Copy link
Member Author

blgm commented Dec 11, 2024

There's cfg.authMiddleware and cfg.additionalMiddleware which are different. The order they are applied in is:

mw := append(append(cfg.authMiddleware, defaultMiddleware(logger)...), cfg.additionalMiddleware...)

So the first auth middleware is applied first, then any additional auth middlewares in the order that they were added, then the default middleware, then any additional middlwares in the order that they were added.

@zucchinidev zucchinidev merged commit d7788d6 into main Dec 20, 2024
5 checks passed
@zucchinidev zucchinidev deleted the chiless branch December 20, 2024 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants