-
Notifications
You must be signed in to change notification settings - Fork 4
feat: SSEMarshaler for browser-consumable streaming responses #92
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
Open
ankurs
wants to merge
5
commits into
main
Choose a base branch
from
feat/sse-marshaler
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
f4069b6
feat: add SSEMarshaler for browser-consumable streaming responses
ankurs cf5fcc8
feat: register SSEMarshaler by default; add DisableSSEMarshaler opt-out
ankurs 5d9cc5e
fix: SSEMarshaler multiline payloads + immutable delimiter
ankurs 1196262
chore: switch govulncheck to symbol scan and bump golang.org/x indirects
ankurs 122a63a
chore: bump go directive to 1.26.3 for stdlib CVE fixes
ankurs File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,38 @@ | ||
| package core | ||
|
|
||
| import ( | ||
| "mime" | ||
| "net/http" | ||
|
|
||
| "github.com/go-coldbrew/core/config" | ||
| "github.com/klauspost/compress/gzhttp" | ||
| ) | ||
|
|
||
| // sseMediaType is the Content-Type advertised by SSEMarshaler and excluded | ||
| // from HTTP compression. | ||
| const sseMediaType = "text/event-stream" | ||
|
|
||
| // newHTTPCompressionWrapper builds the gzhttp wrapper used by initHTTP. It | ||
| // negotiates gzip and (unless disabled) zstd from Accept-Encoding. Pulled out | ||
| // so it can be tested without standing up the full gateway. | ||
| // | ||
| // text/event-stream is excluded via excludeSSEContentTypeFilter — proxies | ||
| // and CDNs buffer compressed SSE responses, defeating real-time delivery. | ||
| func newHTTPCompressionWrapper(cfg config.Config) (func(http.Handler) http.HandlerFunc, error) { | ||
| return gzhttp.NewWrapper( | ||
| gzhttp.MinSize(cfg.HTTPCompressionMinSize), | ||
| gzhttp.EnableZstd(!cfg.DisableZstdCompression), | ||
| gzhttp.PreferZstd(!cfg.DisableZstdCompression && cfg.PreferZstd), | ||
| gzhttp.ContentTypeFilter(excludeSSEContentTypeFilter), | ||
| ) | ||
| } | ||
|
|
||
| // excludeSSEContentTypeFilter wraps gzhttp.DefaultContentTypeFilter to also | ||
| // exclude text/event-stream, so SSE frames are delivered uncompressed and | ||
| // reach the client without intermediary buffering. | ||
| func excludeSSEContentTypeFilter(ct string) bool { | ||
| if mediaType, _, err := mime.ParseMediaType(ct); err == nil && mediaType == sseMediaType { | ||
| return false | ||
| } | ||
| return gzhttp.DefaultContentTypeFilter(ct) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
Add language identifiers to fenced code blocks.
The two new fenced blocks are missing language tags (
MD040) and can fail markdown lint.Suggested fix
-
+go&core.SSEMarshaler{JSONPb: runtime.JSONPb{
MarshalOptions: protojson.MarshalOptions{EmitUnpopulated: true},
}}
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
@README.mdaround lines 526 - 528, The README's fenced code blocks aroundexamples that include core.RegisterHTTPMarshaler, &core.SSEMarshaler{}, and the
&core.SSEMarshaler{JSONPb: runtime.JSONPb{...}} snippet lack language
identifiers and trigger MD040; update each of those triple-backtick blocks to
include the Go language tag (```go) so the examples for
core.RegisterHTTPMarshaler and the SSEMarshaler/JSONPb snippet are fenced as Go
code.
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.
Skipping for this PR. The README is auto-generated by
gomarkdocfrom the Go doc comments — those code samples render without language tags because gomarkdoc reformats them. MD040 is a lint warning, not a CI failure, and the right fix is upstream in gomarkdoc rather than hand-editing the regenerated README (which would just get clobbered on the nextmake doc).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.
@ankurs, understood — makes complete sense. Patching a generated file would just create noise for the nextmake docrun, and the fix belongs in the gomarkdoc template/tooling rather than here.✏️ Learnings added