Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jeffreydwalter opened this issue Sep 17, 2018 · 11 comments
Open

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

jeffreydwalter opened this issue Sep 17, 2018 · 11 comments

Comments

@jeffreydwalter
Copy link
Contributor

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): jeffreydwalter@998bb67)

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
Copy link
Contributor Author

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

@jeffreydwalter
Copy link
Contributor Author

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.

@purehyperbole
Copy link
Member

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

@jeffreydwalter
Copy link
Contributor Author

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

jeffreydwalter pushed a commit to jeffreydwalter/sse that referenced this issue Sep 24, 2018
jeffreydwalter pushed a commit to jeffreydwalter/sse that referenced this issue Sep 24, 2018
jeffreydwalter pushed a commit to jeffreydwalter/sse that referenced this issue Sep 24, 2018
@Muddz
Copy link

Muddz commented Mar 15, 2020

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

@loeffel-io
Copy link

Any update on this?

@purehyperbole
Copy link
Member

@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

@loeffel-io
Copy link

Thanks for the info @purehyperbole

@cryptix
Copy link

cryptix commented Mar 24, 2021

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.

@alevinval
Copy link

alevinval commented Apr 7, 2022

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.

@mapero
Copy link

mapero commented Aug 24, 2022

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.

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

No branches or pull requests

7 participants