-
Notifications
You must be signed in to change notification settings - Fork 759
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
Initial sketch of an SSE JSON serializer helper. #5557
Conversation
|
||
/// <summary>Represents a server-sent event.</summary> | ||
/// <typeparam name="T">Specifies the type of data payload in the event.</typeparam> | ||
public readonly struct SseEvent<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This largely replicates the SseItem<T>
type present in System.Net.ServerSentEvents. I feel we might be able to avoid duplication but that would require taking a dependency on one more NuGet package.
/// <returns>A task representing the asynchronous write operation.</returns> | ||
public static async ValueTask SerializeAsSseAsync<T>( | ||
Stream stream, | ||
IAsyncEnumerable<SseEvent<T>> sseEvents, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider an overload accepting IASyncEnumerable<T>
?
private static readonly byte[] _sseEventFieldPrefix = "event: "u8.ToArray(); | ||
private static readonly byte[] _sseDataFieldPrefix = "data: "u8.ToArray(); | ||
private static readonly byte[] _sseIdFieldPrefix = "id: "u8.ToArray(); | ||
private static readonly byte[] _sseLineBreak = Encoding.UTF8.GetBytes(Environment.NewLine); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the OS-specific line break felt appropriate given that all variants are supported by the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most implementations I've seen always use '\n' regardless of OS.
/// <param name="options">The options configuring serialization.</param> | ||
/// <param name="cancellationToken">The token taht can be used to cancel the write operation.</param> | ||
/// <returns>A task representing the asynchronous write operation.</returns> | ||
public static async ValueTask SerializeAsSseAsync<T>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want this as a public API, I'd rather it be in System.Net.ServerSentEvents. For now it can be a non-public implementation detail from anything that needs to use it. I don't think we should be exposing this publicly from M.E.AI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now it can be a non-public implementation detail from anything that needs to use it.
Do you have any particular use case in mind? Like a specific streamer for IAsyncEnumerable<StreamingChatCompletionUpdate>
or something else?
I don't think we should be exposing this publicly from M.E.AI.
Should I file a proposal in runtime following this shape?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any particular use case in mind? Like a specific streamer for IAsyncEnumerable or something else?
The use case I've been talking about all along, being able to write out the M.E.AI object model as an OpenAI-compatible response, both non-streaming and streaming varieties (this case being the latter).
Should I file a proposal in runtime following this shape?
Sure. But in runtime ideally I'd want it to be something ASP.NET would rely on, so it'd be good to understand what its needs would be.
71b7833
to
528735a
Compare
528735a
to
e1e598d
Compare
Superseded by dotnet/runtime#109832. Will follow up with a PR that adds the dotnet/runtime implementation as a polyfill. |
Creating this as a draft to solicit some initial feedback on the design.
Fix #5546.
Microsoft Reviewers: Open in CodeFlow