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

Complete catalog of client-side events #436

Closed
jwr opened this issue Sep 5, 2023 · 7 comments
Closed

Complete catalog of client-side events #436

jwr opened this issue Sep 5, 2023 · 7 comments

Comments

@jwr
Copy link
Contributor

jwr commented Sep 5, 2023

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 could wrap the data in a custom event myself
  • I could use a complete catalog of sente events to filter out anything that I don't want passed into the dispatcher, if such a catalog exists. I looked at lines 31-35 of sente.cljc, which list "Client-side events", but I'm not sure if that is (and will be) all of them. Also, this list still contains :chsk/recv?
  • I could filter by namespace if I knew (for example) that all sente events will be in the "chsk" namespace
  • or perhaps there is a way to tell that the event came from user code calling :send-fn and I just don't know about it?

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
Copy link
Member

@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
Copy link
Contributor Author

jwr commented Sep 5, 2023

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
Copy link
Member

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
Copy link
Contributor Author

jwr commented Sep 5, 2023

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
Copy link
Member

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

Cheers :-)

@jwr
Copy link
Contributor Author

jwr commented Sep 5, 2023

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
Copy link
Member

ptaoussanis commented Sep 5, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants