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!

[nietsmmar]

Does your fork work in web? :) Thanks for sharing your work!

[tamcy]

@nietsmmar On one hand, It looks like Dart's HttpClient on the web doesn't yet support receiving SSE event (issue: dart-lang/http#337). But, from the same issue, a developer implemented fetch_cilent as a drop-in replacement of HttpClient for the moment.

As for the fork, I didn't take web support into consideration when updating the code because I didn't need it. However, it supports using a custom HTTP client of package:http via the httpClientProvider argument. Since FetchClient implements the HttpClient interface, you can provide a FetchClient instance when constructing SseClient:

  final client = SseClient(
    http.Request('GET', Uri.parse('http://example.com/subscribe'))
      ..headers.addAll({
        'Authorization': 'Bearer (token)',
        'Cache-Control': 'no-cache',
      }),
    httpClientProvider: () => FetchClient(mode: RequestMode.cors),
  );

I just tested and it works. But I didn't do much testing on the web, so more feedback and contributions are needed.

[nietsmmar]

@tamcy Thank you so much. I will try this and report back.

[nietsmmar]

EDIT
@tamcy It works! Thank you so much. There is just one problem for me. I am not sure why leading whitespaces in the data field are removed?

if (fieldValue.startsWith(' ')) {
   fieldValue = fieldValue.substring(1);
}

Because I am streaming chatGPT OpenAI chunks. They often lead with whitespaces. Without them, I just get the text without any whitespaces. Is there a reason why they are removed? Curl doesn't do that for example.

Same problem with new lines at the end of incoming events. Maybe it is this paft of code. Although when editing this line to not remove line breaks, the data weirdly has more whitespace and the line break is still missing.

/// [spec] If the data buffer's last character is a U+000A LINE FEED (LF) character, then remove the last character from the data buffer.
  MessageEvent toMessageEvent() => MessageEvent(
        id: id,
        event: event,
        data: data.endsWith('\n') ? data.substring(0, data.length - 1) : data,
      );

[tamcy]

@nietsmmar The message interpretation is implemented according to what is specified in the standard. Again, quote https://html.spec.whatwg.org/multipage/server-sent-events.html:

And the following example:

If the server side sends the message in a way that expects the first whitespace after the colon be preserved, it is violating the specification and it is the server code, not the client code that should be fixed.

Same for ending line breaks. curl outputs the received message as-is and should not be compared directly.

[nietsmmar]

@tamcy Thank you for the reply. I see. I just forwarded what I get from OpenAI-Completion. I will change my backend then. Thanks again for your great help.

[nietsmmar]

It would be great to return the original error in the client.dart instead of creating a new one like this:

if (response.statusCode != 200) {
    throw Exception('Failed subscribing to SSE - invalid response code ${response.statusCode}');
}

Because now the onError method doesn't show me my original thrown error from my backend to react on it properly.