actix-web icon indicating copy to clipboard operation
actix-web copied to clipboard

Add ConditionOption middleware

Open KoltesDigital opened this issue 3 years ago • 3 comments

PR Type

Feature

PR Checklist

  • [X] Tests for the changes have been added / updated.
  • [X] Documentation comments have been added / updated.
  • [ ] A changelog entry has been made for the appropriate packages.
  • [X] Format code with the latest stable rustfmt.
  • [x] (Team) Label with affected crates and semver status.

Overview

I'm interested in conditionally applying a middleware. But with Condition, I find strange that the middleware has to be created even if the condition is false.

One simple, quite idiomatic improvement would have been to use a lambda to lazily generate the middleware.

But there's a more idiomatic way to do, using Option!

As Transform is defined in another crate, a newtype is required, which I called ConditionOption. Sadly it has to be mentioned for wrap to work.

KoltesDigital avatar Aug 31 '22 10:08 KoltesDigital

This should be added to actix-service where Transform can impl Option<T>

fakeshadow avatar Sep 01 '22 00:09 fakeshadow

That's what I thought at the beginning, then I saw that the latest documentation for Transform mentions impls for Arc and Rc but they've disappeared from the master branch, so I thought that its impls have to be removed.

In the other hand, that would move ConditionMiddleware as well. I don't master your architecture, I can't make such a bold move.

KoltesDigital avatar Sep 01 '22 05:09 KoltesDigital

You can start by making an issue in actix-net repo. We want to get it right since we already know what shoud be done. Rather than just add more duplicated (purpose) code.

fakeshadow avatar Sep 01 '22 07:09 fakeshadow