Better state-handling in citrus ? about citrus HOT 24 CLOSED

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!

from citrus.

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.

from citrus.

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

from citrus.

from citrus.

In custom Citrus impl at work :)

from citrus.

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).

from citrus.

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)

from citrus.

from citrus.

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.).

from citrus.

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

from citrus.

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.

from citrus.

@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.

from citrus.

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 😅 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?

from citrus.

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?

from citrus.

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

from citrus.

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.

from citrus.

That’s exactly how it works! 😄

from citrus.

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 😉

from citrus.

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. 🤗

from citrus.

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?

from citrus.

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

from citrus.

from citrus.

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.

from citrus.

Cool, I'm glad to hear!

from citrus.