-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
feat(gatsby): use graphiql-explorer #14280
Conversation
@@ -0,0 +1,53 @@ | |||
{ | |||
"name": "gatsby-graphiql-explorer", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this probably need better package name - this is kind of express middleware, but not exactly, because it's not used as express.use(someMiddleware())
, instead it is used as graphiqlExplorer(expressApp, params)
as I need to have 2 route handlers (one for html and one for bundle)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's easy to bike shed here--I don't really have a problem with the name. We could also scope it, e.g. @gatsbyjs/graphiql
which is really what we've done here. We tweaked graphiql
with some extra functionality.
@pieh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, left some comments.
In general, I don't necessarily think that:
- Name change should block, nor
- Default GraphiQL query should block
I think this is useful enough to ship. Great work!
@@ -0,0 +1,16 @@ | |||
# gatsby-graphiql-explorer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple and concise 🤷♂ I think this will suffice!
Co-Authored-By: Dustin Schau <DSchau@users.noreply.github.com>
Co-Authored-By: Dustin Schau <DSchau@users.noreply.github.com>
Will try to figure better tests e2e tests tomorrow, not quite sure why failing test is flaking in CircleCi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
e2e-tests/development-runtime/cypress/integration/functionality/graphql-endpoint.js
Outdated
Show resolved
Hide resolved
…y/graphql-endpoint.js Co-Authored-By: Dustin Schau <DSchau@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 Let's merge!
@pieh - question, how does this impact https://www.gatsbyjs.org/docs/using-graphql-playground/ ? Is the GraphQL Playground also updated with this feature? If not, should that be mentioned in the Docs or Blog post? |
Replace current bare graphiql with graphiql-explorer + graphiql
Additional benefit that is not really related to graphiql-explorer, but implemented here:
new graphiql integration doesn't use external resources (currently used
express-graphql
uses https://cdn.jsdelivr.net to get react, react-dom, graphiql etc to render app correctly. This will use local bundle, so you can use it no problem on planes ;)TO-DO:
Closes #11602