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

Dispatcher middlewares #267

Closed
wants to merge 6 commits into from

Conversation

yeraydiazdiaz
Copy link
Contributor

Alternative to #176 inspired by a comment from @tomchristie

I quite like this approach, it's nice and simple. I only see two caveats:

  1. The "middleware" dispatches have different initializers than "pure" dispatchers requiring a next_dispatcher as an argument. It's not really a problem, just find the fact that we're deriving from AsyncDispatch when we're actually using duck/structural typing a bit misleading. Maybe we could formalize it somehow?
  2. The logic to "resolve" the dispatcher can get out of hand and is a mixture of client attributes and send arguments. That being said the auth logic is probably a lot more complicated than other usages we might have.

I'm keen to hear if this approach would help implementing features like proxy support. I think redirection and auth look a lot simpler this way.

@florimondmanca
Copy link
Member

I think there might be ways to reduce boilerplate and improve modularity of middleware dispatchers, but overall this is looking very encouraging! I’ll check out the code and play with it. :-)

@florimondmanca
Copy link
Member

florimondmanca commented Sep 1, 2019

Closed in favor of #268

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