-
Notifications
You must be signed in to change notification settings - Fork 21
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
Better state-handling in citrus ? #34
Comments
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 |
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
<#34 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEt3nqG-FBr80Glo_8zStNoM8G36mEGks5uMyWWgaJpZM4VskpD>
.
|
In custom Citrus impl at work :) |
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). |
I'd keep Note that I say 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) |
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
<#34 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEt3lsQsknYfZ2ss4eR8xDZ7dKqRJiAks5uNAxSgaJpZM4VskpD>
.
|
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 |
Just pushed an update into See example code there, it's one of the possible APIs |
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 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 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. |
@DjebbZ Another implementation of this is now available in Citrus master. It has some of the same shortcomings you already described for See the |
Thanks for tackling the problem. I've read the docs and still have some questions:
|
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? |
Yes the whole state. With the tests I get it now (good tests BTW). One could just |
@DjebbZ Should we keep this issue open or do you think it's sufficiently addressed by |
I'm wondering if citrus should provide a default |
That’s exactly how it works! 😄 citrus/src/citrus/reconciler.cljs Lines 17 to 55 in f6f3bdd
|
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 😉 |
Since all the code in |
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? |
Not sure I understand. Do you want to give me a ping on slack and we can figure it out there? |
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 ***@***.***> 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
<#34 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAS3XXV4LIVOB4KLLBHUGTRN4ZQ3ANCNFSM4FNSJJBQ>
.
|
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 |
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. |
Cool, I'm glad to hear! |
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.
So one can write a single function that returns all the state, or several functions that return parts of it which can just
merge
.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:
In the current design of citrus, it means one potentially has to dispatch two events:
or handle the same event in 2 different controllers.
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:
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:
dispatch -> effect -> multiple reads
orlogic in UI component -> multiple reads
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:API would roughly look like this:
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).
The text was updated successfully, but these errors were encountered: