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

Add support for subscription connectionParams in GraphiQL #548

Merged
merged 11 commits into from
Oct 9, 2017
Merged

Add support for subscription connectionParams in GraphiQL #548

merged 11 commits into from
Oct 9, 2017

Conversation

Glockenbeat
Copy link
Contributor

@Glockenbeat Glockenbeat commented Sep 7, 2017

This PR adds the ability to pass connectionParams to the GraphiQL options in the GraphiQL plugins (solved #452).

TODO:

  • Update CHANGELOG.md with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

@apollo-cla
Copy link

@Glockenbeat: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@DxCx
Copy link
Contributor

DxCx commented Sep 8, 2017

LGTM but i think we need a better naming, at least adding websocket before so it will be
websocketConnectionParams

@Glockenbeat
Copy link
Contributor Author

Would be possible of course, however I tried to stick to the naming which is used for the SubscriptionServer onConnect() method in the authentication example. Should we change it anyways?

@Glockenbeat
Copy link
Contributor Author

Updated the naming as proposed, so it's "websocketConnectionParams" now.

@Stradivario
Copy link

Thank you guys! I was trying to implement some logic with cookies passed from the server.When the authorization header from the first graphiql request was passed to the client on second request for subscription cookies are passed trough socket and then the server verifies the token again.

Waiting for this to be merged :)

@rdickert
Copy link

rdickert commented Sep 29, 2017

I came to suggest this very thing and found it almost done. Awesome!

I have one possible change: Perhaps the item passed to websocketConnectionParams already be stringified rather than stringifying it at the destination. I was setting up some code:

  websocketConnectionParams: {
    authToken: localStorage.getItem('AUTH_TOKEN'),
  },

which is what we use on the client, but since I'm setting this up on the server, localStorage is undefined, and it throws an error. It needs to run this code on the client, so I probably have to be able to pass a string rather than actual objects so I can run this code in the right environment. i.e.,

  websocketConnectionParams: `{
    authToken: localStorage.getItem('AUTH_TOKEN'),
  }`,

We already do this with passHeader.

@martijnwalraven
Copy link
Contributor

@Glockenbeat @DxCx: What do you think of @rdickert's proposal?

@Glockenbeat
Copy link
Contributor Author

I like the idea of being able to use data from the client rather than setting it in the GraphiQL connection settings on the server. Something like that is definitely needed.

Personally I don't really like how the values are passed as a string as they are implicitly executed on the client, but since that is already implemented in the same way for passHeader it should be fine to use here as well.

@Glockenbeat
Copy link
Contributor Author

Would it be possible to merge this PR and iterate on it afterwards? So far the discussion seems to be a bit stuck but I think this could be helpful for some people to work with connectionParams and websockets, even when there might be better implementations in the long run.

@martijnwalraven
Copy link
Contributor

@Glockenbeat: I'd be happy to merge this if other people agree. @DxCx? @rdickert?

@stubailo
Copy link
Contributor

stubailo commented Oct 9, 2017

Yeah let's do it!

@martijnwalraven martijnwalraven merged commit 1874943 into apollographql:master Oct 9, 2017
@mjgallag
Copy link

mjgallag commented Dec 7, 2017

Is there any way to pass an auth token from local storage with websocketConnectionParams as passHeader allows?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants