Skip to content

Conversation

@l5cs
Copy link
Contributor

@l5cs l5cs commented Oct 12, 2025

pr for #501

corrects the handling of reconnect events sent through eventsub websocket in accordance with twitch docs.

i'm making this a draft and looking for feedback because

  • i've spent a couple days looking at my own implementation and it seems to work but i haven't tested locally with twitch cli
  • still no handling of keepalive messages. the handle should timeout and reset the connection
  • the code in general looks ugly

Copy link
Contributor

@Nerixyz Nerixyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for updating the example!

A few comments/suggestions:

  • Leave out the token refreshing. This can be a comment/TODO for the user, but it's a bit distracting.
  • Similarly, get_existing_subscriptions shouldn't be needed if we track the subscriptions properly. The assumption is that the given token is only used by the example.
  • Split the process_message into multiple functions to avoid nesting.
  • Could the state for a connection still live in some struct similar to before?

@Emilgardis
Copy link
Member

I agree with splitting it up more, tried doing a quick review but it was hard piecing together what it's actually doing without spending more time on it.

@l5cs l5cs marked this pull request as ready for review October 14, 2025 19:52
Copy link
Contributor

@Nerixyz Nerixyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The structure looks good. Just a few minor things and some more comments.

Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can merge this, I'm not a huge fan of how it's done but this is more correct. We can improve the example more, I'll experiment with it myself in #477

@Emilgardis
Copy link
Member

Could you rebase the PR please git pull upstream main --rebase? also I see you're on your main branch, this might cause problems when you do the rebase, we'll see.

@l5cs
Copy link
Contributor Author

l5cs commented Oct 22, 2025

seems to work. 👍
sorry for the hold up, i got a bit ill. thanks for taking your time to look into this ❤️

Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

@Emilgardis Emilgardis added this pull request to the merge queue Oct 23, 2025
Merged via the queue into twitch-rs:main with commit a4f36d5 Oct 23, 2025
19 checks passed
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 this pull request may close these issues.

3 participants