Add Server Sent Events #4609
Add Server Sent Events #4609
Conversation
|
I used the TypeScript compiler to convert it to ES5, so I'm sorry if it doesn't look great. |
|
Tested this on TiddlyServer and after fixing this one bug, everything works on TiddlyServer as well, without any changes to TiddlyServer. |
| var events = new EventSource(host + "events/plugins/tiddlywiki/tiddlyweb"); | ||
| events.addEventListener("change", function () { | ||
| $tw.syncer.syncFromServer(); | ||
| }); |
There was a problem hiding this comment.
Would it be a good idea to add some debouncing here to reduce the amount of requests going to the server over a small period of time?
There was a problem hiding this comment.
Another idea - you might want to add an event listener for the error event. Otherwise if you lose the connection to the server, you'll need to reload the entire wiki to reestablish it!
There was a problem hiding this comment.
I think debouncing is going to be necessary to avoid obvious DDOS vectors. Alternatively, perhaps we should disable SSE by default.
|
Hi @Arlen22 - this is great! I've been hoping for support for SSE in TiddlyWiki for a while, and I agree that this would be a good time to add it. I looked at your code and left some ideas I have for improvement - please let me know what you think! Also, you mentioned how you originally wrote this in TypeScript - do you by chance have a TypeScript definition file for TiddlyWiki's API? I think it would be very useful! |
I do not. Because of how often closures are used it is very hard to make it work properly with TypeScript. Plus I don't know how to keep it in sync. It seems like it would be a full time job. I have often wished, though :) |
|
@Jermolene any idea when we can get this in? |
|
@Arlen22 Have you considered lifting this out into a standalone plugin so that others could play with it some more? I'd like to give this a try in my wiki, but I'd feel more comfortable with it as a plugin rather than having to patch the tiddlyweb plugin! |
|
No. Jeremy has said that it would be ideal to have SSE in core, and the other two files are tiddlyweb specific so can't really separate them out. |
| "use strict"; | ||
|
|
||
| /** | ||
| * @param {string} prefix |
There was a problem hiding this comment.
The core doesn't use JSDoc, just plain text for code comments
There was a problem hiding this comment.
I use it to give VS Code the proper intellisense. TiddlyWiki is really hard to work with otherwise.
There was a problem hiding this comment.
Yes, for us familiar with type script, interface with typing is very nice.
| return ServerSentEvents; | ||
| }()); | ||
|
|
||
| exports.ServerSentEvents = ServerSentEvents; |
There was a problem hiding this comment.
The construction used to export the class isn't consistent with the rest of the core, which doesn't use the inner IIFE that returns "ServerSentEvents". It should follow the same approach as, say, the core widget classes.
| this.handler = handler; | ||
| this.prefix = prefix; | ||
| } | ||
| ServerSentEvents.prototype.getExports = function () { |
There was a problem hiding this comment.
The core leaves a blank line between methods
| * @param {*} state | ||
| */ | ||
| ServerSentEvents.prototype.handleEventRequest = function (request, response, state) { | ||
| if (request.headers.accept && request.headers.accept.startsWith('text/event-stream')) { |
There was a problem hiding this comment.
The core doesn't use a space between if and the following parenthesis
| ServerSentEvents.prototype.handleEventRequest = function (request, response, state) { | ||
| if (request.headers.accept && request.headers.accept.startsWith('text/event-stream')) { | ||
| response.writeHead(200, { | ||
| 'Content-Type': 'text/event-stream', |
There was a problem hiding this comment.
The core prefers double quotes for string constants, only using single quotes for strings that need to contain double quotes.
| }; | ||
| ServerSentEvents.prototype.emit = function (response, event, data) { | ||
| if (typeof event !== "string" || event.indexOf("\n") !== -1) | ||
| throw new Error("type must be a single-line string"); |
There was a problem hiding this comment.
We would normally capitalise an error message
| exports.synchronous = true; | ||
| exports.platforms = ["browser"]; | ||
| exports.startup = function () { | ||
| //make sure we're actually being used |
There was a problem hiding this comment.
Comments should be capitalised, with a space after the //
| exports.platforms = ["browser"]; | ||
| exports.startup = function () { | ||
| //make sure we're actually being used | ||
| if($tw.syncadaptor.name !== "tiddlyweb") return; |
There was a problem hiding this comment.
Could we instead install the event handler when we startup the syncadaptor?
There was a problem hiding this comment.
I'm sure we could but I don't really know where. I did it here because that is right after the syncer and sync adapter are initialized. But you could just as easily do it there.
| var events = new EventSource(host + "events/plugins/tiddlywiki/tiddlyweb"); | ||
| events.addEventListener("change", function () { | ||
| $tw.syncer.syncFromServer(); | ||
| }); |
There was a problem hiding this comment.
I think debouncing is going to be necessary to avoid obvious DDOS vectors. Alternatively, perhaps we should disable SSE by default.
|
I'm sure I missed stuff. I don't know really. I've kind of lost interest. Feel free to use it if you feel it's useful. I use VS Code and Typescript for several years now and depend on it for a lot of stuff. So it's kind of hard to work with the internals of TiddlyWiki code because the wrapper functions block the type detection. |
|
@NicolasPetton I appreciate the work here. I'll close mine as it looks like you've taken care of everything. |
This is related to Issue #4608 and also fixes Issue #4605.
To test this, run
node tiddlywiki.js editions/server --listenthen open two pages side-by-side and watch changes propagate in almost real time.