search

sysid / sse-starlette

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

feat: send default ping as comment message #44

Closed eino closed 1 year ago

eino commented 1 year ago

Send default ping payload as a comment line, as recommended by the RFC https://html.spec.whatwg.org/multipage/server-sent-events.html#authoring-notes

Prevents some clients to raise an error when parsing the payload.

paxcodes commented 1 year ago

Have this been tested? It seems to me that this will not achieve what you're aiming to do. ping is an event type. Your change will not transform the ping to a comment line but only change the event field from event: ping to event: :ping

Sending a comment line is just one way to send pings; a custom event type will do the same**. What client are you using? I would think your client should be able to handle any  event type including pings

** See example in MDN, https://developer.mozilla.org/en-US/docs/Web/API/Server-sent_events/Using_server-sent_events

Pax Williams | GitHub https://github.com/paxcodes | Portfolio https://margret.pw | LinkedIn https://www.linkedin.com/in/pax-w/

2 Feb 2023, 06:41 by @.***:

Send default ping payload as a comment line, as recommended by the RFC > https://html.spec.whatwg.org/multipage/server-sent-events.html#authoring-notes

Prevents some clients to raise an error when parsing the payload.

You can view, comment on, or merge this pull request online at:

  > https://github.com/sysid/sse-starlette/pull/44

Commit Summary b284449 https://github.com/sysid/sse-starlette/pull/44/commits/b28444999e0d7e3e6ae6c0e6ae859a2ccf5bb17e> feat: send default ping as comment message File Changes

(> 1 file https://github.com/sysid/sse-starlette/pull/44/files> )

M> > sse_starlette/sse.py https://github.com/sysid/sse-starlette/pull/44/files#diff-470a2bb4505add32ff7977749cfe4299d0625c3de2b168bdbe68878ee19ce55f> (2) Patch Links: https://github.com/sysid/sse-starlette/pull/44.patch https://github.com/sysid/sse-starlette/pull/44.diff

— Reply to this email directly, > view it on GitHub https://github.com/sysid/sse-starlette/pull/44> , or > unsubscribe https://github.com/notifications/unsubscribe-auth/ADIDWNWKSH2DHS34HIS2VJ3WVPBTNANCNFSM6AAAAAAUPDTQJI> . You are receiving this because you are subscribed to this thread.> Message ID: > <sysid/sse-starlette/pull/44> @> github> .> com>

eino commented 1 year ago

Of course it's tested; the client I use fails when trying to parse the default ping message as it's not an expected payload. Maybe there is a need to filter on the event type, I'll check that out. However for now I solved it by sending this ping as a comment, as indicated in the HTML spec.