Better state-handling in citrus ?

[DjebbZ]

Hi,

I want to talk about an idea I have of citrus that would be a breaking change but would IMHO make citrus-based code more expressive, coherent and testable.

The problem

There is a strange difference between server-side and browser-side citrus.

• In the server, a resolver returns all the state necessary at once. A resolver is a map with all the necessary keys of the state filled up with some values. So one can write a single function that returns all the state, or several functions that return parts of it which can just merge.
• In the browser, controllers/event handlers are restricted to operate on a single part/domain of the state. To be more precise, a handler can only read this single part of the state and write to the same single part.

This restriction, while it could look good at first (this is the equivalent of Splitting reducers and combining them in Redux), is overly restrictive, and means that events which originate from the app, from some user interaction maybe must have their semantics tied to one specific part/subdomain of the state.

For a small example, imagine a settings form with one input. Every time a user submit the form, you want to do two things with the state:

• save the text from the input,
• increment a global counter of form submissions.

In the current design of citrus, it means one potentially has to dispatch two events:

:on-submit (fn [_]
            (citrus/dispatch! reconciler :settings :save input-value)
            (citrus/dispatch! reconciler :submissions :inc))

or handle the same event in 2 different controllers.

:on-submit (fn [_]
            (citrus/dispatch! reconciler :settings :settings-form-submitted)
            (citrus/dispatch! reconciler :submissions :settings-form-submitted))

Now, what if you wanted to save only when the global counter is less than 10 ? You can't do it in neither the :settings controller nor the :submissions controller! You have to create an effect just to be able to get a handle on a reconciler so that you can read arbitrary part of the state.

The effect handler would look like this:

(let [new-submissions-counter (inc @(citrus/subscription reconciler :submissions))]
  (citrus/dispatch! reconciler :submissions :inc)
  (if (< 10 new-submissions-counter)
    (citrus/dispatch! reconciler :settings :save input-value)))

Which means an effect handler must be used whereas we do no side-effect, only manipulating state! Or you have to put this code in the UI component itself, which far from optimal. It also means that in these cases our events look more like "RPC" calls. They don't convey the semantics of what happened, but are often named after the how to manipulate the state part.

Expressed more formally, the problem is that:

• writing to multiple parts of the state for one event is not possible, one has to dispatch one event per state slice or split the logic into different controllers.
• reading from several parts of the state for one event is not possible, one has to go either through dispatch -> effect -> multiple reads or logic in UI component -> multiple reads
• logic is spread everywhere, leading to less cohesive, difficult to maintain and hard to test code
• server-side and client-side citrus operate too differently

A proposed solution

Ideally one would dispatch! a single event like :settings-form-submitted, and do all the logic at once. To do that citrus needs to change in breaking ways. Here's what I think is the least breaking way:

• change nothing server-side
• client-side,
  • create a reconciler with a single controller multimethod that receive the whole state and returns only the part that changes
  • (unsure about it) still declares the top-level keys in the state and use this to check state effects.

API would roughly look like this:

;; single multimethod
(citrus/reconciler {:state (atom {:settings nil :submissions 0}) ; same as before, not empty here for the test below
                    :effect-handlers {} ;same as before
                    :controller control
                    :state-keys #{:settings :submissions}})

(defmulti control (fn [event] event))

;; coherent, cohesive
(defmethod control :settings-form-submitted [event [input-value] {:keys [submissions] :as current-state}]
    {:state {:settings input-value
             :submissions (inc submissions)}})

;; always testable
(deftest settings-form-submitted
   (citrus/dispatch-sync! reconciler :settings-form-submitted "some text")
   (is (= "some text" @(citrus/subscription reconciler [:settings]))
       (= 1 @(citrus/subscription reconciler [:submissions]))))

The top-level keys declaration is not mandatory at all to make it work, but it means without this we lose the information. That's why we could also before setting the state check that the keys under the :state effect belong to the set of keys declared.

Pros/cons

Obvious cons: breaking change... It could exist in a fork/new version of citrus though.

Pros: more coherent code, events names only convey the semantics, every state-related change can happen in a single event handler. It would also make it easier to integrate a good logger into citrus, that would display both the event-name and the state before/after à la Redux-dev-tools.

Receiving the whole state isn't a problem, immutability got our back. And by the way this how Redux works out of the box, before you split the reducers.

The rest of citrus doesn't change. I think in terms of code size the change could be quite small.

What do you think ?

PS: sorry if there are errors/typos in this (again) long blob of text. PS 2: pinging @jacomyal, I'd love this input (we already had a similar conversation at work, but I haven't exposed him this precise stuff).

[roman01la]

I agree about passing the whole state into a handler, that’s what I do actually. Not sure about removing multimethods, but since it can be built on top of a single function maybe it makes sense. I have to think how to minimize breakage here

[DjebbZ]

I don't understand "that’s what I do actually". How ?

Le jeu. 2 août 2018 à 18:15, Roman Liutikov notifications@github.com a écrit :

I agree about passing the whole state into a handler, that’s what I do actually. Not sure about removing multimethods, but since it can be built on top of a single function maybe it makes sense. I have to think how to minimize breakage here

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/roman01la/citrus/issues/34#issuecomment-409982811, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEt3nqG-FBr80Glo_8zStNoM8G36mEGks5uMyWWgaJpZM4VskpD .

[roman01la]

In custom Citrus impl at work :)

[DjebbZ]

Another advantage is that side-effects don't need to be tied to a specific controller like it currently is. Just dispatch an event that do the side-effect(s).

[roman01la]

I'd keep :controllers to not break existing code and add :citrus/handler key for that single handler function. How does it sound to you?

Note that I say function, not multimethod. Then you can use that function to dispatch on whatever value you want. The only requirement is that handler function should return an effect.

Example:

(defn handler [[ctrl event & args] state]
  ;; call your mutimethod here or lookup a map, etc.
)

(def r (citrus/reconciler {:state (atom {})
                           :citrus/handler handler}))

(citrus/dispatch-sync! r :counter :init 0)

[DjebbZ]

I think it's a good idea not to break stuff, so yes, sounds good.

PS: Remember, you have a good test suite now 😉. It'll be easy to duplicate the tests for the new single handler and verify it works the same way.

Le ven. 3 août 2018 à 10:39, Roman Liutikov notifications@github.com a écrit :

I'd leave :controllers to not break existing code and add :citrus/handler key for that single handler function. How does it sound to you?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/roman01la/citrus/issues/34#issuecomment-410186618, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEt3lsQsknYfZ2ss4eR8xDZ7dKqRJiAks5uNAxSgaJpZM4VskpD .

[DjebbZ]

Oh, I did not the see the edit with your code sample when I commented. Well, I understand you want to add the feature without breaking stuff (good), but this idea looks to me like lazy and messy workaround (bad). Sorry I don't have better words. The duality will make it complex for both new users and users with an existing codebase.

What I would do instead is have a real multimethod at the new key :citrus/handler like I proposed. To avoid the mess and confusion of having both, you could verify that only one of :controllers and :citrus/handler is used when building the reconciler, and throw and it's not the case. Then users could use the "new" API or the "old" one. Or even better: to ease the transition, allow both, then in core citrus, first call the :citrus/handler and provide yourself a :default implementation that returns a special value like :citrus/no-handler. When no handler exists, fallback to calling the :controllers multimethod and proceed normally. This should ease the transition. After that you're free to decide if and how you want to deprecate controllers (bugfixes, perf patches etc.).

[roman01la]

Just pushed an update into citrus/handler branch https://github.com/roman01la/citrus/tree/citrus/handler

See example code there, it's one of the possible APIs

[DjebbZ]

Well, this implementation allows to have a single event that gets redispatched to as many controllers as we want. What I don't like is the amount of boilerplate it adds (wiring the single handler to controllers yourself, implementing broadcast yourself, and the rest of this paragraph - keep reading). Say I want to dispatch a :some-form-submitted that affects 2 parts of the state and triggers a side effect. I would need to handle it in the handler, dispatch it to the 2 controllers, duplicate the name for the 2 controllers to keep the naming semantics and handle state changes there. Also, where should I triggers the side effect from? Controller A or B?

I think having this separation makes no sense in the ideal design. I have an idea for an implementation that would still be fully backwards-compatible but more involving, in the sense that the various dispatch/broadcast functions would support a new arity with a specific implementation tailored for a real single handler. It wouldn't use the controllers at all.

In some sense this new arity would make it simpler that having several ifs as there is in the current code you wrote, and make it more obvious how a single handler works.

I'd like to give it a try but I'm currently in holidays for the whole month of August. Maybe I'll find some time otherwise it will wait until September.

Thanks for writing some code so fast for this discussion, and sorry for the harsh words I used before.

[martinklepsch]

@DjebbZ Another implementation of this is now available in Citrus master. It has some of the same shortcomings you already described for citrus/handler (i.e. you need to wire controllers and effects yourself) but in the end it allows you to do many things that previously weren't possible (like getting access to the entire app state in your controller methods).

See the :default handler docs for more details.

[DjebbZ]

Thanks for tackling the problem. I've read the docs and still have some questions:

• does it break isomorphism? It seems not since it's a cljs thing but asking for just to be clear.
• I still don't get how it works :sweat_smile: My primary concern is effecting the whole state, but I don't see how this could be useful. Do you have a small but complete example of how you would do that? Maybe a test?

[martinklepsch]

It doesn't break isomorphism and the change should be fully backwards compatible.

There's a test here: https://github.com/clj-commons/citrus/blob/master/test/citrus/custom_handler_test.cljs

When you say effecting the whole state you mean have a controller method write outside it's controller state?

[DjebbZ]

Yes the whole state. With the tests I get it now (good tests BTW). One could just assoc to different parts of the state in one shot. I see how it allows one to customize the handler. You could still have declarative controllers if you wanted and manage side effects in your controllers if you wish. Nice!

[martinklepsch]

@DjebbZ Should we keep this issue open or do you think it's sufficiently addressed by :default-handler?

[DjebbZ]

I'm wondering if citrus should provide a default default-handler that behaves just like regular controllers (declarative state and side effects) while leaving the possibility for anyone to override it (or not plug it all). It could be a function that the user has to opt-into by using the :default-handler option , basically just expose it and explain how to wire it in the docs. It could make life easier for people while leaving the possibility to use a custom one if needed.

[martinklepsch]

That’s exactly how it works! 😄

https://github.com/clj-commons/citrus/blob/f6f3bdd4d89f2311a0e22281fb627fdc82289829/src/citrus/reconciler.cljs#L17-L55

[DjebbZ]

Oh I didn't see, sweat! Then I'm ok to close the issue. The extra mile would be to add a unit test for the default-handler, just to make sure it doesn't break anything... Just saying 😉

[martinklepsch]

Since all the code in master already uses Citrus' default handler all the unit tests already use this new code path 🙂 I'm going to rename the :default-handler option to :citrus/handler as Roman did in his initial exploration and then hopefully we can cut a proper release with this soon. In the meantime any testing of master would certainly be welcome. 🤗

[DjebbZ]

Sure, this proves that the code is backward compatible. But if I'm not mistaken not unit test explicitly makes use of the new default-handler?

[martinklepsch]

Not sure I understand. Do you want to give me a ping on slack and we can figure it out there?

[DjebbZ]

No time for Slack, sorry. Plus I've taken the time to seriously read the code and finally understand my confusion.

I missed the fact that the default handler was implicitly used by default when none was provided. I first thought it was completely optional, whereas it's mandatory to have a handler, it's just that by default the handler behaves like citrus normally behaves to avoid breaking changes.

My initial understanding was that the default handler was an addition to the normal behavior, whereas it's a replacement. I thought one could just use a default handler for the events that need the whole state and citrus would automatically fallbacks to the normal behavior in case it doesn't find a new-style handler.

But it doesn't matter, I've just figured out that I could write a handler my self that does just this: try to find a new-style handler, and if it doesn't exist just call citrus.reconciler/citrus-default-handler to fallback to the normal case. Am I correct?

On Wed, Apr 22, 2020, 8:29 PM Martin Klepsch notifications@github.com wrote:

Not sure I understand. Do you want to give me a ping on slack and we can figure it out there?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/clj-commons/citrus/issues/34#issuecomment-617952846, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAS3XXV4LIVOB4KLLBHUGTRN4ZQ3ANCNFSM4FNSJJBQ .

[martinklepsch]

But it doesn't matter, I've just figured out that I could write a handler my self that does just this: try to find a new-style handler, and if it doesn't exist just call citrus.reconciler/citrus-default-handler to fallback to the normal case. Am I correct?

That's correct. Initially my intention was to actually provide a "fallback-handler" type option similar to what you are intending to do. But then I realized that this would be trivial to implement on top of a more general option like what we have now. Side note: this is also the reason this is called default-handler, which I'll change before release.

[DjebbZ]

Thanks a lot for your valuable contribution and help! This feature will definitely solve the biggest problem I have with citrus. With it, citrus will become a very good state management library IMHO.

[martinklepsch]

Cool, I'm glad to hear!