search

sysid / sse-starlette

BSD 3-Clause "New" or "Revised" License
505 stars 36 forks source link

feat: Send pings as comments by default #53

Closed paxcodes closed 1 year ago

paxcodes commented 1 year ago

Given 3 users who have made a comment on the ping events in #51 and #44 -- it might be best to send pings as comments by default, that way clients don't have to deal with them out-of-the box.

To achieve this, we can change the ping from an event data to an event comment as seen on this changeset,

image

However, the package does not seem to be encoding the events properly when a comment is included in the event. Examples,

  1. If the event only includes a comment, ServerSentEvent(comment="a comment"), then
    • expected encoded event is : a comment\r\n\r\n
    • but actual encoded event is: : a comment\r\n
  2. If the event includes data and a comment, ServerSentEvent(data="some data", comment="a comment")
    • expected encoded event is : a comment\r\ndata: some data\r\n\r\n
    • but actual encoded event is: : a comment\r\n

If I understand the spec correctly (screenshot below) I think the expected data specified above is correct. Let me know otherwise! image (from https://html.spec.whatwg.org/multipage/server-sent-events.html)

Testing

  1. Test cases that include comments in events, were added and they pass
  2. Existing tests pass
  3. Manual E2E test shows that a ping has been sent as a comment; and since it's sent as a comment, it's not parsed by the browser as a message or data image
sysid commented 1 year ago

I was about to merge, but when I run the tests locally make test they are failing. Would you mind having a look.

paxcodes commented 1 year ago

Will do. Attempting to install using pipenv but pipenv is stuck on locking dev dependencies.

22 Apr 2023, 02:51 by @.***:

I was about to merge, but when I run the tests locally > make test> they are failing. Would you mind having a look.

— Reply to this email directly, > view it on GitHub https://github.com/sysid/sse-starlette/pull/53#issuecomment-1518582636> , or > unsubscribe https://github.com/notifications/unsubscribe-auth/ADIDWNWDMMCNZHYBEUCGAATXCOSZ5ANCNFSM6AAAAAAXDRVZRU> . You are receiving this because you authored the thread.> Message ID: > <sysid/sse-starlette/pull/53/c1518582636> @> github> .> com>

paxcodes commented 1 year ago

@sysid This is ready for review. The change is more extensive as I ran into some issues when an event includes a comment. See PR description for more info.