Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Send pings as comments by default #53

Merged

Conversation

paxcodes
Copy link
Contributor

@paxcodes paxcodes commented Apr 19, 2023

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
  1. 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

@paxcodes paxcodes marked this pull request as ready for review April 19, 2023 05:43
@paxcodes paxcodes marked this pull request as draft April 19, 2023 05:59
@sysid
Copy link
Owner

sysid commented Apr 22, 2023

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

@paxcodes
Copy link
Contributor Author

paxcodes commented Apr 22, 2023 via email

@paxcodes paxcodes force-pushed the feature/send-pings-as-comments-by-default branch from 5d862e9 to 252cdf8 Compare April 30, 2023 03:34
@paxcodes paxcodes marked this pull request as ready for review April 30, 2023 04:04
@paxcodes
Copy link
Contributor Author

@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.

@sysid sysid merged commit 74732c0 into sysid:master May 2, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants