sse icon indicating copy to clipboard operation
sse copied to clipboard

This library doesn't seem to follow the EventSource spec.

Open jeffreydwalter opened this issue 6 years ago • 11 comments

Hello, me again... I noticed that this library just reads a line and parses it, looking for a <field>: and then sends that as an event. This is not correct according to the EventSource spec.

Is there a reason you chose to implement this library in the current form, or were you trying to comply with the standard?

If so, I've made some changes to a local copy of your library to make it follow the standard (although it's not 100% yet) and would be happy to create a PR.

Here's a sample gist that demonstrates the issue: https://gist.github.com/jeffreydwalter/a01b14c51a6c0fc405278f49e0d99db2

Here's a fork that has my proposed changes (doesn't 100% follow the standard yet): https://github.com/jeffreydwalter/sse/commit/998bb6797a1720029c2c8655ccf3c5ed7fc96d86)

When I run the above gist, I get:

go run blah.go 
ID: 
EVENT: message
DATA: 
ERROR: 
ID: 
EVENT: 
DATA: {"status":"connected"}
ERROR: 
^Csignal: interrupt

What I would expect (according to how I understand the EventSource standard is that the Event should be like this:

$ go run blah.go 
ID: 
EVENT: message
DATA: {"status":"connected"}
ERROR: 

jeffreydwalter avatar Sep 17 '18 14:09 jeffreydwalter

This library is also missing the retry semantics and the handling of the Last-Event-ID doesn't seem right.

jeffreydwalter avatar Sep 17 '18 14:09 jeffreydwalter

I've used this Python sse client library in production code and have looked at it's code quite a bit. It seems to follow the standard pretty closely and would be a good library to model.

jeffreydwalter avatar Sep 17 '18 14:09 jeffreydwalter

Hi!

As the library was released with a server implementation and was only intended to be used in conjunction with one another, there was never any emphasis on conforming to the EventSource Spec per say.

Given that this library is seeing more use than I anticipated, any changes to improve adherence to ES spec is a good thing.

So far the changes on your branch look great! :)

Once you've got something that you're happy to submit, please open a PR and i'll do some testing

Cheers

purehyperbole avatar Sep 17 '18 16:09 purehyperbole

Going to leave this open since retry isn't fully implemented.

jeffreydwalter avatar Sep 18 '18 19:09 jeffreydwalter

@purehyperbole What is the status of this? Isn't it implemented fully now to be complient with EventSources specifications?

Muddz avatar Mar 15 '20 22:03 Muddz

Any update on this?

loeffel-io avatar Feb 24 '21 14:02 loeffel-io

@loeffel-io The implementation adheres to the SSE specs on everything with the exception of supporting the retry field. We use exponential backoff to determine when to reconnect, so it never made sense to support that usecase where that value is actually used, which isn't everywhere.

It's something that could be added easily however, or you can set your own reconnect strategy by setting ReconnectStrategy

purehyperbole avatar Feb 24 '21 15:02 purehyperbole

Thanks for the info @purehyperbole

loeffel-io avatar Feb 24 '21 16:02 loeffel-io

The implementation adheres to the SSE specs on everything with the exception of supporting the retry field.

Great news! I suggest changing the issue title to reflect this new state. I got a bit worried for a moment when evaluating if we should use this for a new project.

cryptix avatar Mar 24 '21 09:03 cryptix

FYI you can find spec compliant and optimised decoder here maybe you find it useful. You can use the decoder with any io.Reader, so it should not be hard to integrate. You could ignore the rest of the primitives.

alevinval avatar Apr 07 '22 12:04 alevinval

Hi. We have noticed there is a mandatory query parameter "stream" in the server implementation. Without providing the parameter, the connection will not be established. This seems to be something that is not specified by the spec and will prevent to use this library with other sse client implementations. Is it possible to make this parameter optional or completely remove it?

Thanks.

mapero avatar Aug 24 '22 08:08 mapero