Embed the Hijacker interface in intercepting writer#1093
Embed the Hijacker interface in intercepting writer#1093wicknicks wants to merge 4 commits intogo-kit:masterfrom
Conversation
Signed-off-by: Arjun Satish <arjun@confluent.io>
transport/http/server.go
Outdated
|
|
||
| type interceptingWriter struct { | ||
| http.ResponseWriter | ||
| http.Hijacker |
There was a problem hiding this comment.
This isn't safe — if this field is nil, h, ok := w.(http.Hijacker) will succeed, but calling h.Hijack() will panic, as h will be nil.
|
http.Hijacker is one of five common interfaces which http.ResponseWriters are upgraded-to using the interface upgrade pattern — why just solve for this one? |
|
hey, @peterbourgon thanks very much for the suggestions. yes, it makes sense to accommodate all five interfaces. such a switch case block was what I had in mind for that, but using |
Signed-off-by: Arjun Satish <arjun@confluent.io>
|
@peterbourgon made the changes. let me know what you think. thanks in advance! |
|
btw, I'm not very fluent in golang. does the |
Signed-off-by: Arjun Satish <arjun@confluent.io>
|
@peterbourgon any thoughts on this approach? thanks in advance! |
|
@peterbourgon what do you think? Can this be merged? 🙏 |
peterbourgon
left a comment
peterbourgon
left a comment
There was a problem hiding this comment.
With that change, OK. Not super excited about the additional dependency, but 🤷
transport/http/server.go
Outdated
| "net/http" | ||
|
|
||
| "github.com/felixge/httpsnoop" | ||
|
|
There was a problem hiding this comment.
Nit: rm the blank line and re-goimports :)
Signed-off-by: Arjun Satish <arjun@confluent.io>
|
@peterbourgon removed the extra newline. |
|
@peterbourgon do you think we can merge this now, and release a new tag? many thanks in advance! |
|
This is a big change to a core package. I'll merge it now, so we can get some beta-testing time, so to speak. |
|
Oof! What am I saying. Can you please add some tests for this? 😇 |
|
Happy to re-review with proper test coverage. |
…m stdlib. successor to go-kit#1093 and closes go-kit#1092.
3 participants
This PR embeds the Hijacker interface in kit's interceptingWriter. Having an hijacker embedding allows applications to use websockets. looks here for discussion around this: golang/go#26937
Addresses issue #1092.