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

feat(gatsby): use graphiql-explorer #14280

Merged
merged 25 commits into from
May 27, 2019
Merged

feat(gatsby): use graphiql-explorer #14280

merged 25 commits into from
May 27, 2019

Conversation

pieh
Copy link
Contributor

@pieh pieh commented May 23, 2019

Replace current bare graphiql with graphiql-explorer + graphiql

Kapture 2019-05-23 at 21 50 03

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:

  • Write proper README
  • Discuss new package name (?)
  • Consider crafting nice default query - for the most part empty query filled with comments that will target Gatsby users (and maybe graphiql-explorer)
  • Add some e2e tests to to increase confidence in this (and any future changes to this)

Closes #11602

@pieh pieh requested a review from a team as a code owner May 23, 2019 19:54
@@ -0,0 +1,53 @@
{
"name": "gatsby-graphiql-explorer",
Copy link
Contributor Author

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)

Copy link
Contributor

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.

@DSchau
Copy link
Contributor

DSchau commented May 23, 2019

@pieh I wrote a README for you, but can't push to the branch! Could you give collaborators (aka me) access?

DSchau
DSchau previously approved these changes May 23, 2019
Copy link
Contributor

@DSchau DSchau left a 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
Copy link
Contributor

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!

packages/gatsby-graphiql-explorer/package.json Outdated Show resolved Hide resolved
packages/gatsby-graphiql-explorer/src/app/app.js Outdated Show resolved Hide resolved
@pieh
Copy link
Contributor Author

pieh commented May 24, 2019

Will try to figure better tests e2e tests tomorrow, not quite sure why failing test is flaking in CircleCi

@pieh pieh removed the status: WIP label May 24, 2019
DSchau
DSchau previously approved these changes May 24, 2019
Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌 Let's merge!

@Kevin-Hamilton
Copy link

@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?

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.

Consider integrating graphiql-explorer
3 participants