refactor: graphiql template shared across servers - #49
Conversation
Sorry, something went wrong.
| variables: {{variables|tojson}}, | ||
| operationName: {{operation_name|tojson}}, | ||
| defaultQuery: {{default_query|tojson}}, | ||
| headerEditorEnabled: {{header_editor_enabled|tojson}}, |
There was a problem hiding this comment.
might be worth adding shouldPersistHeaders as well, to decide whether or not the headers tab should persist it's values in localstorage by default
Sorry, something went wrong.
There was a problem hiding this comment.
Can you point out where it is defined? So far I am looking at the current renderGraphiQL.js implementation at the express-graphql package and I didn't saw shouldPersistHeaders.
Sorry, something went wrong.
There was a problem hiding this comment.
My bad, it's in the 1.0.0 readme:
https://github.com/graphql/graphiql/tree/1.0.0/packages/graphiql#props
Sorry, something went wrong.
| "graphiql_version": graphiql_version, | ||
| "graphiql_html_title": graphiql_html_title, | ||
| "query": data.get("query"), | ||
| "variables": data.get("variables"), |
There was a problem hiding this comment.
we also have a new headers string prop that allows you to set the default headers value if you want :)
Sorry, something went wrong.
There was a problem hiding this comment.
I just noticed latest changes on renderGraphiQL.js have the headers option but it is passed at the graphQLFetcher function whereas these variables are passed to the ReactDom function.
Sorry, something went wrong.
There was a problem hiding this comment.
you can see the new props here: https://github.com/graphql/graphiql/tree/1.0.0/packages/graphiql#props
the headers that are passed in the fetcher function are the ones from the editor. the headers react prop is for pre-populating the headers editor
Sorry, something went wrong.
| <link href="//cdn.jsdelivr.net/npm/graphiql@{{graphiql_version}}/graphiql.css" rel="stylesheet" /> | ||
| <script src="//cdn.jsdelivr.net/npm/promise-polyfill@8.1.3/dist/polyfill.min.js"></script> | ||
| <script src="//cdn.jsdelivr.net/npm/unfetch@4.1.0/dist/unfetch.umd.js"></script> | ||
| <script src="//cdn.jsdelivr.net/npm/react@16.13.1/umd/react.production.min.js"></script> | ||
| <script src="//cdn.jsdelivr.net/npm/react-dom@16.13.1/umd/react-dom.production.min.js"></script> | ||
| <script src="//cdn.jsdelivr.net/npm/graphiql@{{graphiql_version}}/graphiql.min.js"></script> | ||
| <script src="//cdn.jsdelivr.net/npm/subscriptions-transport-ws@0.9.16/browser/client.js"></script> | ||
| <script src="//cdn.jsdelivr.net/npm/graphiql-subscriptions-fetcher@0.0.2/browser/client.js"></script> |
There was a problem hiding this comment.
Based on 1.0.0 readme, looks like the promise-polyfill cdn isn't needed, right? @acao
Also I noticed that the subscription-transport-ws package has been archived so I am not sure if there are alternatives up to date for supporting subscriptions. Do you have any idea about it?
Sorry, something went wrong.
There was a problem hiding this comment.
promise-polyfill has everything to do with whether you want IE support
subscription-transport-ws is still an active project and should be used. we removed it from our example and never got around to providing a new subscriptions example
Sorry, something went wrong.
There was a problem hiding this comment.
Oops, I linked the wrong package, I meant graphiql-subscriptions-fetcher which still shows up on the use it with graphiql readme section of subscription-transport-ws.
Sorry, something went wrong.
There was a problem hiding this comment.
Also, by looking at the subscription-transport-ws, I am a bit worried about the latest release as it was at March 2019 and there are several PRs and issues waiting for maintainers to be answered so not sure if it is active at another branch or fork but would be good to know.
Sorry, something went wrong.
jkimbo
left a comment
There was a problem hiding this comment.
Couple of small comments but otherwise this looks great!
Sorry, something went wrong.
jkimbo
left a comment
There was a problem hiding this comment.
👍 looks good @KingDarBoja
Sorry, something went wrong.
Fixes #48.
EDIT
I forgot to add the typing-extensions on the setup file in order to support the TypedDict 🤦♂️