Skip to content

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 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!

@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

4 participants