Skip to content

Conversation

GlenDC
Copy link
Collaborator

@GlenDC GlenDC commented Jul 8, 2025

Motivation

retry was the only field for which we did not add a space

Solution

@jplatte
Copy link
Member

jplatte commented Jul 8, 2025

Thanks for the PR! Pinging @SabrinaJewson for review since I still have close to no knowledge of SSE.

@SabrinaJewson
Copy link
Contributor

SabrinaJewson commented Jul 9, 2025

Seems reasonable either way to me. FWIW, Spaces are mandatory for all the other fields because the field value itself may start with a space (including json in case the user uses RawValue for example), and retry is the only field where it’s not necessary. But having consistency also makes sense.

@GlenDC
Copy link
Collaborator Author

GlenDC commented Jul 9, 2025

Seems reasonable either way to me. FWIW, Spaces are mandatory for all the other fields because the field value itself may start with a space (including json in case the user uses RawValue for example), and retry is the only field where it’s not necessary. But having consistency also makes sense.

strictly speaking, https://html.spec.whatwg.org/multipage/server-sent-events.html, defines spaces as always optional. If the first character of the value is a space it is to be removed, regardless of the field.

That said any other field in Axum's SSE implementation uses a space so there's little reason not to do it for retry. On top of that all other implementations I am aware of use a space, and some test suites rely on this space to be there.

Therefore I would argue to just add a space here as well. It doesn't hurt, it makes it more consistent, aligns with other implementations and doesn't do harm anywhere.

@jplatte jplatte merged commit 617594c into tokio-rs:main Jul 9, 2025
18 checks passed
@jplatte
Copy link
Member

jplatte commented Jul 9, 2025

I wonder whether this should have a changelog entry 🤔

@GlenDC
Copy link
Collaborator Author

GlenDC commented Jul 9, 2025

I wonder whether this should have a changelog entry 🤔

are you planning to do a release soon? how often are these releases? Otherwise I might, if you are ok with it, want to still squeeze in the original issue I posted (about exposing the writer)

@jplatte
Copy link
Member

jplatte commented Jul 9, 2025

No immediate plans for a release, no. Feel free to include the changelog entry in your next PR. Thanks!

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.

3 participants