Add support for SSE comments - #478
Conversation
`ServerSentEvent` now carries a `comments` field, so that comment lines (those starting with `:`) can be both parsed and serialised. Per the WhatWG specification such lines are ignored by clients, which makes them the idiomatic keep-alive: they stop proxies from dropping an idle connection without dispatching an event to the application. Binary compatibility with 1.7.18 is preserved in the same way as for `ContentTypeRange`: the old-arity constructor, `copy` and `apply` are kept alongside the new ones. Closes #379 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| id: Option[String] = None, | ||
| retry: Option[Int] = None, | ||
| comments: List[String] = Nil | ||
| ): ServerSentEvent = new ServerSentEvent(data, eventType, id, retry, comments.flatMap(_.split(LineTerminators))) |
There was a problem hiding this comment.
Should the invariant be that comments never contains line terminators? Right now only this apply enforces it: new ServerSentEvent(...) — and Scala 3 codecs derived via Mirror, which construct through fromProduct — can still create comments = List("a\nb"). toString output stays safe either way, but such an instance doesn't round-trip (parse gives the split form) and isn't equal to the same event built via apply.
So: do we enforce the invariant everywhere, or allow both forms?
Also, maybe we should have property-based tests for the parse/serialise round-trip — they'd catch cases like this.
There was a problem hiding this comment.
Ah, I missed fromProduct. In this case I think allowing both forms is better. comments would then be a plain record of what was passed, and toString would keep splitting when writing.
Enforcing it everywhere would make the model throw, which doesn't fit safeApply/unsafeApply here, and would turn a newline in a comment into an exception inside derived decoders.
So I'd remove the split from apply and keep it in comment(...)`, so the factory still hands back one entry per line.
That would also remove the re-splitting in parse.
On property tests: currently there's no scalacheck in the dependencies, so we can consider just adding a table-driven round-trip test over the special shapes ("", "\n", "\r", "\r\n", "a\nb", "ping\n\n", multi-item lists). But happy to add property tests if you don't mind adding that dependency :)
There was a problem hiding this comment.
If needed, no problem in adding a test dep
| /** An event consisting of a single comment line. Such events are ignored by clients, and can be used to keep the | ||
| * connection alive, so that it isn't dropped by proxies. | ||
| */ | ||
| def comment(comment: String): ServerSentEvent = ServerSentEvent(comments = List(comment)) |
There was a problem hiding this comment.
A comment that is only line terminators disappears entirely: "\n".split("\r\n|\r|\n") is an empty array, so comment("\n") equals ServerSentEvent() and serializes to "" — a keep-alive built from such a string sends nothing. Meanwhile comment("") gives ": ". Worth a test pinning what should happen here.
| def parse(event: List[String]): ServerSentEvent = { | ||
| event.foldLeft(ServerSentEvent()) { (event, line) => | ||
| if (line.startsWith("data:")) combineData(event, removeLeadingSpace(line.substring(5))) | ||
| if (line.startsWith(":")) event.copy(comments = event.comments :+ removeLeadingSpace(line.substring(1))) |
There was a problem hiding this comment.
Every branch here calls copy, which goes through the 5-arg apply — so the whole accumulated comments list is re-split on every parsed line. That's always a no-op (parse input is already split into lines), and with the :+ append it's quadratic in comment count per event. A copy that forwards to the constructor would avoid it.
| } | ||
|
|
||
| object ServerSentEvent { | ||
| private val LineTerminators = "\r\n|\r|\n" |
There was a problem hiding this comment.
String.split with a multi-char pattern recompiles the regex on every call (master's split("\n") hit the single-char fast path). Since this runs per comment in apply and again in toString, consider precompiling: private val LineTerminators = java.util.regex.Pattern.compile("\r\n|\r|\n"), then LineTerminators.split(s).
2 participants
Fixes #379.
ServerSentEventgains acomments: List[String]field, so comment lines (those starting with:) round-trip through bothparseandtoString.ServerSentEvent.comment("ping")builds the keep-alive frame the WhatWG specification recommends sending every 15 seconds or so, to stop legacy proxies from dropping an idle connection.Design notes
List[String]rather thanOption[String], because a single event block may carry several comment lines.There is deliberately no assertion that a comment and data aren't both set (as considered in the issue discussion). The specification ignores comments per line, not per event, so a block such as
is valid and still dispatches
abc. A guard would reject valid streams.Binary compatibility
Preserved with the same approach already used for
ContentTypeRange: the old-arity constructor,copyandapplyare kept alongside the new ones.Note one source-level (not binary) change: downstream positional patterns like
case ServerSentEvent(d, e, i, r)now need a fifth_. This is inherent to adding a field and is not visible to MiMa.Migration
Comment-only blocks previously parsed to
ServerSentEvent(). A keep-alive: pingnow parses toServerSentEvent(comments = List("ping")), so code that skipped such events by comparing against an empty event silently stops skipping them:isCommentOnlyis true when an event carries no data, event type, id or retry — only comments, if any.Equality checks are the case to watch, because they keep compiling. The positional-pattern equivalent,
case ServerSentEvent(None, None, None, None), cannot regress silently: it fails to compile until a fifth_is added.