binaryminds / react-native-sse

Event Source implementation for React Native. Server-Sent Events (SSE) for iOS and Android 🚀
https://www.npmjs.com/package/react-native-sse
MIT License
174 stars 27 forks source link

Add support for alternative line endings (fix problem with sse-starlette) #45

Closed EmilJunker closed 3 months ago

EmilJunker commented 4 months ago

This is a ~(still experimental)~ implementation to support alternative line endings when parsing the response text. For example, the python library sse-starlette uses Windows-style line endings by default which causes issues for some users.

Surfaced in https://github.com/binaryminds/react-native-sse/issues/41#issuecomment-1942023815

@jackadair @manish181192 Could you please check if this implementation works for you when you connect to a sse-starlette backend configured with the default line ending \r\n? You can test it out with npm install EmilJunker/react-native-sse#line-ending-support

jackadair commented 4 months ago

Hey @EmilJunker just tested this out and appears to be working correctly 👍

EmilJunker commented 4 months ago

Hey @EmilJunker just tested this out and appears to be working correctly 👍

@jackadair Fantastic, thanks 🙏🎉

EmilJunker commented 4 months ago

I have rebased this branch onto master and resolved the conflicts. From my side, this is ready to be merged.

wojciechkrol commented 4 months ago

Hello @EmilJunker :) I love your idea. Please take a look at my commit: https://github.com/binaryminds/react-native-sse/pull/45/commits/c26d708e4bbe251d756d6bf73f830b423f689a96. I've refactored your code a bit and made the lineEndingCharacter configurable. Also, I've added an error throw for cases where the line ending autodetection fails to find the suggested line ending character.

EmilJunker commented 4 months ago

@wojciechkrol Your changes look good to me. The only thing I'm not sure about is throwing that exception when the newline character cannot be detected. I do think it's a good idea to inform the user when this happens since the alternative would be that no events ever get triggered and the user has no clue why. But the problem is that there might be edge cases where after the first onReadyStateChange, the response text simply doesn't contain a newline yet because it only consists of one line. In that case, thowing an exception immediately could be problematic. One possible solution could be to count the number of unsuccessful detections, and if it's more than 10 we throw the exception...

wojciechkrol commented 4 months ago

@EmilJunker Each server event must include a line-ending character. Without it, the event is considered invalid.

Refer to the Server-Sent Events section in the HTML specification for more details: https://html.spec.whatwg.org/multipage/server-sent-events.html.

Additionally, I discovered the reason for the presence of "id" in our code. It serves to reset the Last-Event-ID. I will reintroduce it into the code.

PS: Using console.warn might be enough. It can alert developers if a line-ending character is missing, without stopping the application.

EmilJunker commented 4 months ago

Refer to the Server-Sent Events section in the HTML specification for more details: https://html.spec.whatwg.org/multipage/server-sent-events.html.

Additionally, I discovered the reason for the presence of "id" in our code. It serves to reset the Last-Event-ID. I will reintroduce it into the code.

@wojciechkrol Thank you for this useful link. Now I understand the logic regarding the Last-Event-ID buffer. I have added a commit that makes the code a bit easier to understand by merging the two cases into one: https://github.com/binaryminds/react-native-sse/pull/45/commits/ae0d77463eb3a66c8c472ac5587d62a7a96deae4

The behaviour is still the same as before, only improved. Examples for id values that will be stored in the buffer:

id: myid634
id:myid2303

Examples for id values that will reset the buffer:

id:
id