Skip to content
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

Implement default-handler option to allow full customization of event-handling #50

Closed
martinklepsch opened this issue Jan 30, 2020 · 4 comments · Fixed by #52
Closed

Comments

@martinklepsch
Copy link
Collaborator

martinklepsch commented Jan 30, 2020

EDIT 2020-03-09: This thread discusses the introduction of a :default-handler option – please share your feedback/experiences if you've used it.


Hi all,

We’re currently looking into adapting Citrus so that all controllers methods can access the state of other controllers. In our particular use case we found that we often end up passing central information around in events such as the current users ID. This adds complexity to components because components then need to get this data out of the :users controller first.

We're interested in either something that would allow us to have "shared state" between controllers or a way that would allow us to access individual controllers' state from the methods of a different controller.

There's a few different options I've been mulling over, would be curious what others thing about those.

1. Pass entire reconciler state as 4th argument to multimethods

  • Probably a breaking change
  • Should be hidden behind a config flag I assume

Handlers would take the fourth argument

(defmethod control :foo
  [_ event controller-state reconciler-state]
  ,,,)

2. Default handler option

This is an option I'm very curious about because I think it makes Citrus very versatile and allows existing codebases to gradually migrate out of the predefined controller design while maintaining backwards compatibility. This is not to say that the controller structure is bad. But codebases grow and knowing that it's possible to break out when needed can be great for peace of mind. I know we are at a stage where we have some long-term concerns around building on top of Citrus but a rewrite is just not something that we'll prioritize over product work.

The basic idea is that whenever dispatch! is called with a controller name that doesn't exist in the reconciler it will call a default handler with the following args:

(default-handler reconciler-instance controller event-key event-args)

This would allow user-land code to basically do anything. For instance we could remove some of our controllers from the :controllers map and implement a default-handler like this:

;; let's assume we only need different behavior for the :user controller
(defn default-handler [reconciler ctrl event-key event-args]
  (if (= :user ctrl)
    ;; we pass the entire reconciler state as a fourth argument
    ;; to make everything pluggable there could also be a function that
    ;; processes effects returned by the function below, e.g.
    ;; (citrus/handle-effects reconciler (user/control ,,,))
    (user/control event-key event-args (:user @reconciler) @reconciler)
    (throw (ex-info "Not implemented." {}))))

This implements the suggestion in 1. but possibilities are endless. We could implement an interceptor chain on top of this for instance, which I believe isn't possible with the current controller/multimethod based approach.

I'm not sure what or if this would break with regards to other Citrus features but I think it would be amazing if Citrus had an escape hatch like this that would allow implementing more complex handlers than what's possible with multimethods.

Other solutions to the general problem of accessing central data

Use broadcast! more
  • Use broadcast! to make central pieces of information available to multiple controllers
  • Would require each controller to implement an appropriate handler
Custom effect to mirror data to multiple controllers
  • Implement a :shared-state effect that writes data to every controller in the reconciler
  • Would totally work but feels a little clumsy
@martinklepsch
Copy link
Collaborator Author

It seems that this commit 61210a3 makes a change in the same direction (passing the entire state to every handler). Appears to be unreleased so far and also breaking in terms of API compatibility.

@roman01la
Copy link
Collaborator

@martinklepsch I like the idea with the default handler. Would you want to put an exploratory PR? Perhaps you would want to revert 61210a3 before making changes.

@martinklepsch
Copy link
Collaborator Author

I've started to work on this just now. One question: In the queue-backed dispatch! implementation, why is an anonymous function added to the queue instead of the raw data?

(queue-effects!
queue
[cname event #((get controllers cname) event args (get %1 cname) %2)])

@roman01la
Copy link
Collaborator

@martinklepsch Perhaps just implementation detail. Feel free to change it.

@martinklepsch martinklepsch changed the title Give handlers access to state of every controller / default-handler Implement default-handler option to allow full customization of event-handling Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants