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

Reitit default middleware #114

Merged
merged 25 commits into from
Aug 3, 2018
Merged

Reitit default middleware #114

merged 25 commits into from
Aug 3, 2018

Conversation

ikitommi
Copy link
Member

@ikitommi ikitommi commented Jul 22, 2018

new module metosin/reitit-middleware with common / handy ring-middleware, aiming to be on par with compojure-api.

Review the decisions:

exception-middleware

  • in compojure-api, the middleware took options map with just :handlers key. As there are no other top-level keys, the handlers-map is the only arg taken. I think this is better this way. There is also new :reitit.ring.middleware.exception/wrap key which can be used for example to log all errors - like ring middleware, but for exception handlers, takes 3 args instead of the normal 2 (handler being the first one). With the old options-syntax, this could be the other top-level key in options besides the :handlers, but a special key gives better ergonomics I think.

multipart-middleware

muuntaja

  • the middleware do not take any options, but they read the :muuntaja key from route data. If that is not set, nothing happens (without any warning!)
    • => one needs to set both the middleware and the muuntaja instance to get it working. We could set the muuntaja instance to default one if it's not set, but that would mean it's always enabled for all paths. Currently meta-merge doesn't support removing keys easily - if we inlined meta-merge, we could use the default muuntaja instance if that is not explicitly set (the common case) and people could set it to nil for certain routes which don't require the functionality.
      => I think the explicit :muuntaja key is good.

naming

  • module: reitit-middleware or reitit-ring-middleware or something different?
  • ns'es: reitit.ring.middleware.exception or just reitit.middleware.exception?

@ikitommi ikitommi changed the title Reitit middleware (WIP) Reitit default middleware Aug 2, 2018
@ikitommi
Copy link
Member Author

ikitommi commented Aug 3, 2018

reviewed by @jarppe

@ikitommi ikitommi merged commit b7302a2 into master Aug 3, 2018
@opqdonut opqdonut deleted the reitit-middleware branch March 3, 2023 12:17
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.

1 participant