Add SSE change notifications#196
Conversation
|
(Just from reading the description) This seems like a fine thing to add! Why not use websockets instead of SSE? I thought SSE was deprecated but apparently not, so consider this a curiosity :) You're correct that the version ID isn't actually useful to a listener -- the only useful version for the listener is the child of the version they have. So yeah, why not avoid leaking that information! |
|
Oh, I started with sockets, but wanted to keep it as minimal as possible, and since it doesn't need two-way communication, decided to just use SSE, since it's one way. I think once you move to sockets, it might make sense to do more with them? I'm not against websockets, just didn't want to introduce them without having a reason to. btw. appreciate this server so much, I'm working on a static web app, with a wasm version inside and it's such a nice way to get other people to use my taskwarrior setup (still a bit raw around the edges, but getting there) and this has taken care of 90% of the work. ps. removed the versions_ids, it's much simpler now, and as useful ;) |
djmitche
left a comment
There was a problem hiding this comment.
Sorry for the long delay on reviewing this. I didn't forget but have been very busy!
It looks like this needs a merge conflict resolved, and the below comment addressed, but after that I'm happy to merge!
You mentioned using WASM. Is that in the browser? What storage are you using there?
| } | ||
|
|
||
| pub(crate) fn notify(&self, client_id: ClientId) { | ||
| let event = ChangeEvent { client_id }; |
There was a problem hiding this comment.
Hm, even the client_id is redundant here -- it was supplied in a header to subscribe. Does it make sense to just make this an empty JSON object, since the HTTP stream already includes event: version?
Hi,
I'm not sure if this makes sense for the main server, but I wanted to add it for myself and making a pull request just in case it's interesting. It's (behind a flag, disabled by default), allows any client to listen to version changes for a given client_id, and get instant notifications. I don't see a reason to add more events here, but just in case it's a "version" event.
will get you an SSE event
This could be made more robust by adding :keep-alive as heart-beat, but just wanted a minimal version that doesn't do anything fancy.
I've been using this for the weekend, and it seems like there's no obvious issues.
The only downside (why it's behind a flag) is that it leaks versionIds if you know a clientId. You don't really need the versionId for basic task sync calling, so we could just remove it and keep just the clientId in the version event, which is straight forward