Align Client SSE support to HTML specification #3312
Conversation
|
|
|
|
||
| override def spec: Spec[TestEnvironment with Scope, Any] = | ||
| suite("ServerSentEventClientSpec")( | ||
| test("Line breaks only payload does not emit events") { |
There was a problem hiding this comment.
Postman will emit empty events, chromium does not. Specification says not to.
| |""".stripMargin | ||
| assertEvents(events, Chunk.empty) | ||
| }, | ||
| test("Successive line breaks do not emit extra events") { |
There was a problem hiding this comment.
Similar to the other test, Postman inserts empty events.
| }.mkString("", "\n\n", "\n\n") | ||
| assertEvents(events, Chunk.fill(3)(ServerSentEvent(data = "data"))) | ||
| }, | ||
| test("Event type without data is not emitted") { |
There was a problem hiding this comment.
Postman emits the event, Chromium does not. Specification says not to emit.
| |""".stripMargin | ||
| assertEvents(events, Chunk.single(ServerSentEvent(data = "", retry = Some(1.millisecond)))) | ||
| }, | ||
| test("Event retry that is not an integer is not emitted") { |
There was a problem hiding this comment.
Postman will emit events with provided retry value. Browser ignores them. Specification says to ignore.
disordered
requested review from
jdegoes,
vigoo,
kyri-petrou and
987Nabil
as code owners
| @@ -42,12 +46,12 @@ final case class ServerSentEvent[T]( | |||
| data: T, | |||
| eventType: Option[String] = None, | |||
| id: Option[String] = None, | |||
| retry: Option[Int] = None, | |||
| retry: Option[Duration] = None, | |||
There was a problem hiding this comment.
Please add a constructor, that takes Option[Int] for compatibility.
Also adjust the docs of the param here.
There was a problem hiding this comment.
Added it, though, I realised I missed an edge case, when a negative int value is passed. So, this constructor ends up silently filtering it out as well.
EDIT: Removed again.
The compiler cannot decide which constructor to use when optional values are not provided, e.g. ServerSentEvent("some data"). Named constructor would work, but doesn't address backwards compatibility.
There was a problem hiding this comment.
Please add back the constructor with the Int, but without default values.
| ) { | ||
|
|
||
| def encode(implicit binaryCodec: BinaryCodec[T]): String = { | ||
| val sb = new StringBuilder | ||
| binaryCodec.encode(data).asString.linesIterator.foreach { line => | ||
| binaryCodec.encode(data).asString(StandardCharsets.UTF_8).linesIterator.foreach { line => |
There was a problem hiding this comment.
Please use zio.http.Charsets
disordered
force-pushed
the
align-sse-to-html-spec
branch
2 times, most recently
from
ad9f59c to
119d15b
Compare
disordered
force-pushed
the
align-sse-to-html-spec
branch
from
119d15b to
f818490
Compare
This should (mostly) align SSE support on the
Clientto HTML specification.Fixes:
retryvaluesdatafield valuesBreaking:
retryis nowDurationparsed as millisecondsHandling of
retryHTML specification leaves a lot of room to interpretation for edge cases on how to dispatch events. For browsers it says to not emit events if
datais not specified, however, chromium based browsers consume such events and adjust their retry frequency accordingly. So, this implementation emits such events too withdata = "". It is not obvious, however, that non-string codec has to handle this case.Handling of various line endings
Chromium browsers and Postman handle all versions (Windows, Linux, OSX) of line endings. However, the specification explicitly states only
\nshould be treated as separator. I considered aligning with the browser implementation, butZPipeline.splitLinesonly supports Windows and Linux line endings. It could probably be achieved through some sort of chunk andString.linesIteratormapping.