Skip to content
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

adding requestHeaders to the websocket client #832

Closed
wants to merge 2 commits into from
Closed

adding requestHeaders to the websocket client #832

wants to merge 2 commits into from

Conversation

manute
Copy link

@manute manute commented Aug 14, 2019

Adding requestHeaders argument to the websocket client funcs to have the possibilty of passing request Headers .

For example, if you need to pass other authorization header like X-Auth-Token or/and other headers like Sec-WebSocket-Key , Sec-WebSocket-Versionthis PR opens that possibility.

Example:

client.Websocket(query, http.Header{
     "X-Auth-Token": {"validToken"},
     "Sec-WebSocket-Key": {"SGVsbG8sIHdvcmxkIQ=="},
     "Sec-WebSocket-Version": {13},
}

@coveralls
Copy link

coveralls commented Aug 14, 2019

Coverage Status

Coverage remained the same at 61.817% when pulling 8106c2f on manute:master into 5c644a6 on 99designs:master.

@manute
Copy link
Author

manute commented Aug 15, 2019

hmm the lint fails with

client/websocket.go:56:46: bodyclose: response body must be closed (bodyclose)
	c, resp, err := websocket.DefaultDialer.Dial(url, reqHeaders)

but that body is being closed inside the Subscriptions.Close func and I've not touched any code there 🤔 , any idea @vektah ?

@vektah
Copy link
Collaborator

vektah commented Aug 16, 2019

hmm the lint fails with

try a rebase, I fixed a few of these ~0.9.2 ish.

@vektah
Copy link
Collaborator

vektah commented Aug 16, 2019

PR message could use more explaining why this would be useful, and probably tests/docs on showing how it would be used.

fixing tests

renaming better
@manute
Copy link
Author

manute commented Aug 16, 2019

try a rebase, I fixed a few of these ~0.9.2 ish.

I did and the same results 😢

PR message could use more explaining

Added. Let me know if more doc or tests are needed , although the type added is pretty clear

@vektah
Copy link
Collaborator

vektah commented Aug 19, 2019

Hmm yeah, it's not failing on master but something about your change is confusing the linter.

Lets just skip it, this is just the test client.

c, resp, err := websocket.DefaultDialer.Dial(url, reqHeaders) //nolint:bodyclose

@vektah vektah mentioned this pull request Sep 16, 2019
2 tasks
@vektah
Copy link
Collaborator

vektah commented Sep 16, 2019

Please a look at #861 and see if it covers your use case

@manute
Copy link
Author

manute commented Sep 16, 2019

yeah I think with https://github.com/99designs/gqlgen/pull/861/files#diff-3c0b8ea9d09776a0db9bab04cf0a6ea7R32 it can be added the headers to the websocket requests.

I'll close the issue in lieu of that PR.
Thanks @vektah ,

@manute manute closed this Sep 16, 2019
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.

None yet

3 participants