refactor: graphiql template shared across servers #49
Conversation
KingDarBoja
added
the
type: maintenance
| 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
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.
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
| "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 :)
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.
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
| <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?
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
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.
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.
Fixes #48.
EDIT
I forgot to add the🤦♂️typing-extensionson the setup file in order to support the TypedDict