taoensso / sente

Realtime web comms library for Clojure/Script
https://www.taoensso.com/sente
Eclipse Public License 1.0
1.74k stars 194 forks source link

Complete catalog of client-side events #436

Closed jwr closed 1 year ago

jwr commented 1 year ago

I upgraded from Sente 1.17.0 to 1.19.0 and it took me a while to track down the breakage ā€” which is entirely my fault, it's what I get for not reading the release notes carefully :-) What broke my app was that my data sent from the server was no longer a :chsk/recv event (#319). I pass my data straight to the :send-fn, which no longer wraps it.

I am facing a slight issue right now, because my client application handles :chsk/state and :chsk/handshake, and then passes anything in a :chsk/recv to a "dispatcher", a pub/sub-style system. Now that :chsk/recv is no longer there, it needs to pass everything else into the dispatcher, which isn't great: for sente events, there will be no one to pick those up from the core.async channels. So, I'd rather not pass events that are unknown to my application code.

There are several possible solutions that I can see:

I realize this is not a proper bug report, but it definitely is an "issue", albeit small. I wanted to ask for guidance before I work around this, perhaps this use case wasn't considered when making the changes. Closing as "WONTFIX" is fine, I'll just wrap the data in a custom event myself.

ptaoussanis commented 1 year ago

@jwr Hi Jan! Sorry to hear about the trouble upgrading.

As a first step, just double checking that you're aware that the old wrapping behaviour can be easily retained by changing the wrap-recv-evs? option value?

The details are under Change 1/4 in the migration guide.

Does this maybe help?

jwr commented 1 year ago

No problem at all, the release notes were very clear, I just didn't read them carefully šŸ™‚

Changing the wrap-recv-evs? value does help, if you intend to keep that option around forever. It doesn't if you're thinking about deprecating it in the future. I'm looking for guidance here towards a long-term solution, fixing things today is easy.

ptaoussanis commented 1 year ago

No worries, fully understood šŸ‘

Will definitely keep the option around, it's handy to have and I always use it myself (I prefer the wrapping, personally).

jwr commented 1 year ago

Thank you, that is the kind of guidance I was looking for. In that case, the "complete catalog of events" isn't necessary, I'll just re-enable the wrapping using the wrap-recv-evs? option and the issue can be closed. Many thanks for your help and for continued development and maintenance of sente!

ptaoussanis commented 1 year ago

You're very welcome! Please feel free to ping if you run into any other issues, I'm happy to help.

Cheers :-)

jwr commented 1 year ago

Well, one last semantic nitpick: :chsk/ws-ping also gets wrapped. I would expect it to not be wrapped, as this is the same kind of "infrastructure" event like :chsk/state or :chsk/handshake ā€” it's not an event that I'm sending.

ptaoussanis commented 1 year ago

I believe that's unintentional, so a bug - thanks for mentioning! I'll aim to check the relevant code this week in case there's any other related issues.