Skip to content

Add support for SSE comments - #478

Open
magdzikk wants to merge 16 commits into
masterfrom
sse-comments
Open

Add support for SSE comments#478
magdzikk wants to merge 16 commits into
masterfrom
sse-comments

Conversation

@magdzikk

@magdzikk magdzikk commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Fixes #379.

ServerSentEvent gains a comments: List[String] field, so comment lines (those starting with :) round-trip through both parse and toString. 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 than Option[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

: keep-alive
data: abc

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, copy and apply are 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 : ping now parses to ServerSentEvent(comments = List("ping")), so code that skipped such events by comparing against an empty event silently stops skipping them:

events.filter(_ != ServerSentEvent())  // no longer filters out keep-alives
events.filter(!_.isCommentOnly)        // use this instead

isCommentOnly is 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.

`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>
@magdzikk
magdzikk marked this pull request as ready for review August 28, 2026 14:44

@adamw adamw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Automated review by Claude (posted by @adamw). Main finding: comments are not split on newlines in toString. Details inline.

Comment thread core/src/main/scala/sttp/model/sse/ServerSentEvent.scala Outdated
Comment thread core/src/main/scala/sttp/model/sse/ServerSentEvent.scala Outdated
Comment thread core/src/main/scala/sttp/model/sse/ServerSentEvent.scala Outdated
Comment thread core/src/test/scala/sttp/model/sse/ServerSentEventTest.scala
Comment thread core/src/test/scala/sttp/model/sse/ServerSentEventTest.scala Outdated
Comment thread core/src/main/scala/sttp/model/sse/ServerSentEvent.scala Outdated
Comment thread core/src/main/scala/sttp/model/sse/ServerSentEvent.scala Outdated
Comment thread core/src/main/scala/sttp/model/sse/ServerSentEvent.scala Outdated
Comment thread core/src/main/scala/sttp/model/sse/ServerSentEvent.scala Outdated
Comment thread core/src/main/scala/sttp/model/sse/ServerSentEvent.scala Outdated
Comment thread core/src/main/scala/sttp/model/sse/ServerSentEvent.scala
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.

ServerSentEvent type does not match the WhatWG specification WRT comments

2 participants