Add SseFormatter #109832
Add SseFormatter #109832
Conversation
|
Note regarding the |
1 similar comment
|
Note regarding the |
src/libraries/System.Net.ServerSentEvents/src/Resources/Strings.resx
Outdated
Show resolved
Hide resolved
| public static partial class SseFormatter | ||
| { | ||
| public static System.Threading.Tasks.Task WriteAsync(System.Collections.Generic.IAsyncEnumerable<System.Net.ServerSentEvents.SseItem<string>> source, System.IO.Stream destination, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } | ||
| public static System.Threading.Tasks.Task WriteAsync<T>(System.Collections.Generic.IAsyncEnumerable<System.Net.ServerSentEvents.SseItem<T>> source, System.IO.Stream destination, System.Action<System.Buffers.IBufferWriter<byte>, System.Net.ServerSentEvents.SseItem<T>> itemFormatter, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } |
There was a problem hiding this comment.
This ordering of IBufferWriter then SseItem looks wrong to me. We generally do source and then destination; the source here is the SseItem and the destination is the IBufferWriter.
There was a problem hiding this comment.
I was thinking of how serializers typically order their parameters, e.g. JsonConverter.Write accepts writer first and value second.
There was a problem hiding this comment.
I believe that we intentionally flipped the order in the callback to be (source, destiantion).
Notably, your PR here does not match what was approved.
There was a problem hiding this comment.
Notably, your PR here does not match what was approved.
Yep, this is exploring possible amendments in the callback. Another one is passing the wrapping SseItem<T> instead of just T. Will notify fxdc once we're settled on the shape.
There was a problem hiding this comment.
I believe that we intentionally flipped the order in the callback to be (source, destiantion).
While technically the value is the source and the writer is the destination, this ordering is not what we commonly use in the context of serialization. For example:
- JsonConverter.Write(Utf8JsonWriter, T)
- JsonSerializer.SerializeAsync(Stream, T)
- DataContractSerializer.WriteObject(XmlWriter, object)
Given that the delegate does represent a serialization callback, I would suggest we use the order as presented in this PR.
There was a problem hiding this comment.
https://youtu.be/ehUiLj2KvCg?t=5583
It started as writer first, then got flipped. There was no objection. And, wow, I hate listening to myself.
If you want to change it, you can bring it back for discussion.
There was a problem hiding this comment.
I was hoping we could get this resolved quickly via email. If not, I will begrudgingly change this back to the order as originally approved.
src/libraries/System.Net.ServerSentEvents/src/System/Net/ServerSentEvents/SseItem.cs
Show resolved
Hide resolved
…s.resx Co-authored-by: Stephen Toub <[email protected]>
Fix #109294.