gotham icon indicating copy to clipboard operation
gotham copied to clipboard

RFC: remove StateData trait

Open lucacasonato opened this issue 5 years ago • 4 comments

Currently to be able to store something in State, the data needs to implement the StateData trait. This StateData trait is really just an alias for Any + Send. It would be really nice if implementing this trait would not be needed. Here are some reasons why:

  1. Not technically needed. This abstraction is not actually needed. As you can see in this branch, the StateData can be removed and replaced with direct Any + Send without issues. There is thus no technical reason for the existence of this trait.
  2. Unnecessary boilerplate. To be able to store a struct in gotham::state::State, you need to either add the StateData proc-macro to the declaration, or manually implement gotham::state::StateData. This is an extra line of code that is not needed.
  3. Forces unnecessary wrapper structs. If you want to store a structure from a crate outside of yours in State, you need to wrap it with of a wrapper struct just so you can implement StateData. This adds unnecessary bloat.

I propose that the StateData trait and StateData proc-macro are removed. All methods on State and FromState that currently take a StateData would take any Any + Send instead.

More benefits:

  • This would also be one less thing that users need to learn about when starting out with Gotham.
  • Removes ~150 LOC from gotham and examples.

lucacasonato avatar Oct 23 '20 21:10 lucacasonato

I just looked through the history of gotham and it looks like StateData was introduced together with State, so it's not a leftover from a previous refactor. Does anyone know what the initial reason was to introduce that trait?

msrd0 avatar Oct 23 '20 21:10 msrd0

Considering #494, removing StateData might not be something we'd want to do. If a middleware sometimes puts type Foo into your state and sometimes doesn't, we could map Option<Foo> to State::try_take<Foo>. This would be impossible if Option<Foo> also implemented StateData (or, equivalently, we removed the trait).

msrd0 avatar Nov 03 '20 15:11 msrd0

This would be impossible if Option<Foo> also implemented StateData

How so? You could still make the opinionated assumption that Option is mapped to try_take. Possibly you could even force this in the type system (ie not let the user store Option in state).

Personally I think not being able to store non crate structures directly in the state is a far greater annoyance.

lucacasonato avatar Nov 03 '20 20:11 lucacasonato

How so? You could still make the opinionated assumption that Option is mapped to try_take.

Well, impl<T: Any> FromState for T using take and impl<T: Any> FromState for Option<T> using try_take would clash because Option<T> also implements Any (but does not implement StateData).

I'll wait to see an actual implementation, maybe there's a clever trick that I didn't think of that makes this possible.

msrd0 avatar Nov 03 '20 20:11 msrd0