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

Expose SSE events parser (internal/parser) #39

Open
tmaxmax opened this issue Jul 13, 2024 · 3 comments
Open

Expose SSE events parser (internal/parser) #39

tmaxmax opened this issue Jul 13, 2024 · 3 comments

Comments

@tmaxmax
Copy link
Owner

tmaxmax commented Jul 13, 2024

Hey, I was searching around for only an SSE events parser - encoder/decoder. This was the only library I could find that isn't ancient - thanks for your work!

I'm using an openapi spec which generates ready to use API calls from client side - which is why I am not using the client in go-sse (it would take time to make it play well with openAPI generated code). However, I'm interested in using the code inside internal/parser. I could also help you with exposing it if you had a few ideas in mind.

I saw that the code in https://github.com/donovanhide/eventsource/blob/master/decoder.go, was much simpler and shorter than what you have written up - is there any advantage that I'm missing to your implementation? Please do let me know!

Thanks!

Originally posted by @Pramodh-G in #7 (comment)

@tmaxmax
Copy link
Owner Author

tmaxmax commented Jul 13, 2024

Hi there and thank you for your interest in go-sse! I've created an issue based on your reply to the roadmap so we have a space focused on this discussion only. It's also better for discoverability by others who may have the same question.

First of all, I assume you're looking to decode/encode a wire format representation of an SSE event. go-sse already provides some utilities for that in the exposed API:

Wouldn't these satisfy your use-case or are they too high level? I'm not familiar with what OpenAPI generates – should the already exposed APIs not work for you I could provide better assistance if you wish to elaborate more on how you'd plan to use the internal/parser.

The reason why I'm asking this is because at the moment I wouldn't jump the gun and expose internal/parser as I'd still like to have the flexibility to change the low-level internals without compatibility considerations.

As for the comparison with the code in eventsource, the parser of go-sse closely follows the specification. At a first glance, eventsource doesn't handle line endings and BOM properly – the spec mentions that the line ending can be any of CR/LF/CRLF. go-sse's parser also doesn't drop comments when reading sse.Messages, which may or may not be important to you. Lastly the internal parser is built in a way that it doesn't allocate more than the user allows it to – it can be used (and is used throughout the code) in a zero-alloc manner. Specifically, internal.Parser uses the same buffer for all events parsed and internal.FieldParser does no allocations. In comparison, eventsource allocates for each line.

All in all, go-sse follows the spec, is more flexible and has better performance characteristics.

Finally, I want to thank you for bringing up OpenAPI – it could be an interesting idea to explore a way to integrate go-sse into its generation stack somewhere in the future!

Let me know if this was of help and how I could assist you further!

@Pramodh-G
Copy link

Sorry for the late reply! I've been a bit busy :')

Like you have the WriteTo method, I wanted a ReadFrom method that'd read from a io.Reader or io.ReadCloser. This would ideally require an interface that can read more than a message at a time - so sse.Message.UnmarshalText() might not be the most ideal candidate. I see that internal/parser already has some utilites that do this.

There is SubscribeEvent of the client, but that exposes and underlying http connection - whereas i'd like a raw reader :)

From more of a contributing perspective - I wanted to understand how you want to take this project further. Do you want to improve performance? do you want more UT converage? More integration support (I see that you already have some pub-sub interfaces!)

It would help if we can align so I can contribute to open items!

Thank you so much for the reply :)

@Pramodh-G
Copy link

Disclaimer: While I want to be contributor to the repository, I am definitely not the owner, so I might be wrong/ opinionated here :) . I'm definitely open to having a discussion and reaching a consensus! I feel the structs might need to look like:

  1. Message struct, and methods for that struct that parse text/binary + write to a writer. From your reply above - you already seem to have them :)!

  2. A MessageReader / MessageWriter struct that can read/ write mutiple messages if required from a io.Reader/ io.Writer. We might need this to be separate from the write methods for messages because we would then batch writes instead of writing one message at a time - that might lead to performance boosts.

  3. Both a SSE Client/Server, can use these MessageReader, MessageWriter to write and read SSEs both on client and server. I see that there are both an Event and a Message struct that do contain some shared fields. I do see that an Event is for storing client side state, and a Message is for a server side - but we might get cleaner interfaces when an Event uses a Message internally to parse fields, and some extra fields for the client side intricacies of sse.

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

2 participants