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

Add option to unwrap automatically :chsk/recv events #151

Closed
ebellani opened this issue Jul 24, 2015 · 6 comments
Closed

Add option to unwrap automatically :chsk/recv events #151

ebellani opened this issue Jul 24, 2015 · 6 comments

Comments

@ebellani
Copy link
Contributor

I'm handling server>user pushes with something of this nature:

(defmulti event-msg-handler :id)

(defmethod event-msg-handler :chsk/recv
  ;; default custom push event. Unwraps the true message that, for
  ;; now, comes wrapped in sente.
  [{:as ev-msg :keys [?data]}]
  (let [[message-type message-payload] ?data]
    (event-msg-handler {:id    message-type
                        :?data message-payload})))

Is there an option for not having to do that?

@danielcompton
Copy link
Collaborator

There isn't an option for doing that presently. I'm not speaking for Peter, but he generally prefers simple primitives that can be composed over specific solutions. In this case you have one piece of message handling code that unwraps a Sente message and that's all that's required. It doesn't seem too bad to me, and keeps the interface and implantation code on Sente's side clean.

I may be misunderstanding you though?

@ptaoussanis
Copy link
Member

Hi Eduardo,

Daniel's correct - no option for automatic unwrapping atm.

(BTW thanks a lot for helping out recently @danielcompton, really appreciate it!)

Re: maybe adding an option... have been on the fence about that for a while...

As Daniel mentioned, motivation for the wrapping was originally to keep the top-level [id ?data] message 100% trustworthy (no risk of pollution from application-level messages, etc.).

Thinking about it now, we could probably achieve the same result with a validator on the :<server channel to just protect the :/chsk namespace...

Have some time this morning; will fiddle with it a bit and get back to you.

Cheers :-)

@ptaoussanis
Copy link
Member

Have added a client-side make-channel-socket! :wrap-recv-evs? option (defaults to true for now, you'll want to set it to false). Pushed to Clojars as [com.taoensso/sente "1.6.0-RC1"]

Let me know if that does the trick? Cheers :-)

@ebellani
Copy link
Contributor Author

@danielcompton, thanks for the answer. The reason why I suggested this change is that the current behavior forces everyone to implement the same piece of code in order to get server>user working. So I'd argue that by adding an option (and then turning it into the default later, as @ptaoussanis has done) reduces the cognitive load of the users by reducing the surprise factor of the library.

@ptaoussanis, seems nice, thanks for the work.

@danielcompton
Copy link
Collaborator

That makes sense. In our case we need to have the recv events all going to one multi method so we can handle them uniformly by dispatching them to re-frame.

@ptaoussanis
Copy link
Member

@ebellani Yeah, as Daniel mentioned - it's not unusual to want a different handler for :chsk/recv events; depends on how you've got your event handling structured. Both preferences are quite reasonable now that the :<server channel's protected.

Okay to close this issue?

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

No branches or pull requests

3 participants