On changing the project architecture and fixing bugs

[tamcy]

First off, thank you for your package.

Long story short: I found your package useful, but the current implementation doesn't suit my need, and I was in urgent need of implementing something with SSE. So I copied the code locally, hacked into it, and finished my job first. Now I would like to contribute my changes back.

After forking and trying to update the code into your original repo, I feel hesistated, because I have made substantial changes to the original library, and there is no backward compatibility. So I would like to know your take on this before continuing, or submitting a pull request.

Here is my updated repo: https://github.com/tamcy/dart_sse_client/tree/tamcy-dev

Example can be found here (~~README has not been updated~~ Update: now it's updated): https://github.com/tamcy/dart_sse_client/blob/tamcy-dev/example/main.dart

It's not finished for a pull request, but the code should work.

The changes are aimed to:

1. resolve architectural issues,
2. fix bugs,
3. add test, and
4. improve coding style, like making the package structure and namings more "Dartish".

Here is the longer summary:

Fixing architectural issues

Currently, calling subscribeToSSE() twice will cause the original http.Client being lost, and it's not possible to close the connection of previous http clients (not to mentione there's no way to close a stream). Which is why I made the SSE client an instance class.

I see SseClient something similar to but not the same as an HTTP client. An HTTP client sends a request and returns a response, while an SSE "client" should act more like a wrapper class that encapsulates the details of an SSE connection. With this in mind I have moved the connection details to the constructor. In my design, an SseClient instance will only be responsible of managing the connection of a single request. User should create a new SseClient instance for a different requests.

Also, while an SSE client needs a http.Request object for http.Client, I think it should not be opinionated on how the request object is constructed and the body format (the current library is json-encoding it). So instead of asking for the request method, URL, header and body, the updated version now asks for an http.Request object. This also elminates the need for an SSERequestType enum.

Fixing bugs

I found that the current implementation didn't quite follow the specification on message parsing, especially on how the white spaces and line breaks are handled. Imagine when the message is in plain text and you need to compare the string data. This has been fixed in my fork.

My fork also checks for the response's Content-Type according to the spec:

if res's status is not 200, or if res's Content-Type is not text/event-stream, then fail the connection. This may not be a bug, but I think it's still essential to have such checking. I once connected to a wrong path (actually the path is correct, but there is a server problem on vhost configuration), the response is wrong, but since the response code is 200, the original code just accepted the connection (despite the content type being text/html). Of course, there is no SSE event received whatsoever, and I spent quite some time to figure out the cause.

Coding style fixes

I have also updated the code to make it more Dart-like, at least as per my understanding. For instance, I have separate the published event object from the one that is used for buffering. To me, the buffering object is an internal representation which, while very similar to the dispatch event in structure, should not be exposed to users. In particular, the fields of the emitted message should be immutable.

I have also updated the CHANGELOG.md file so that it shows the changes in descending order. I believe this is a more common practice.

This version should close #10, #18, and replaces pull requests #7, #11 and #15.

Others

~~One missing feature in the updated package is the ability to auto reconnect when fail, because I didn't need it. I am not sure if I can take the time to do it, but I will wait for your response first.~~

Update: Auto reconnection can be achieved using the new AutoReconnectSseClient class.

Besides, I'd also like to suggest changing the package name to something like "dart_sse_client" as the package doesn't depend on Flutter. Also, using a branch name like dev instead of uat should be less confusing.

Thanks for you time!