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-graphiql-explorer): Implement CodeExporter #17120

Merged
merged 11 commits into from Sep 2, 2019

Conversation

@herecydev
Copy link
Contributor

commented Aug 27, 2019

Description

Add's graphiql-code-exporter into gatsby-graphiql-explorer allowing the user to choose between 3 graphql snippets which can be copied/pasted.

Examples

Page query template
image

useStaticQuery Hook
image

StaticQuery Component
image

Related Issues

Fixes #14476

@herecydev herecydev requested a review from gatsbyjs/core as a code owner Aug 27, 2019
@herecydev herecydev changed the title Implement CodeExporter into graphiql feat(gatsby-graphiql-explorer): Implement CodeExporter Aug 27, 2019
@pieh

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

That's very cool!

What do you think about adding 4th snippet for creating pages inside gatsby-node.js?

@pieh

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

Another note I would like to make is about query names (which are MyQuery in example screenshots): Gatsby requires query names (if provided) to be unique. I can imagine workflows when you scaffold multiple pages using GraphiQL explorer -> code exporter that you could end up with multiple queries with same name.

Gatsby also allow users to skip query names (and those will be generated under the hood) - so maybe we could try to strip actual query names from code snippets? It wouldn't be prettiest solution, but doing some regular expression replacement could work here?

Something along the lines of:

getQuery(arg).replace(/query\s.+{/gmi, `query {`)

which I did quick very quick check that it could work

@KyleAMathews

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

Amazing! 🙏

export default () => {
const data = useStaticQuery(graphql\`
${getQuery(arg)}

This comment has been minimized.

Copy link
@herecydev

herecydev Aug 30, 2019

Author Contributor

Minor formatting issue is that the first line gets spaces added, but following lines don't. Is it worth padding the lines with a couple of spaces to keep things nicely formatted?

@herecydev

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

@pieh I've made some further changes if you can take a look. With regards to how errors in the query are handled. The library handles it before running the snippets:

image

@pieh pieh self-assigned this Sep 2, 2019
@pieh

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

I committed some minor fixes and expanded a bit createPages snippet to:

  • be able to handle potential query errors
  • show how to actually create pages from results of query - this is tough, because this is pretty abstract and that part isn't reflecting actual query, but rather is blueprint for how this need to be done. Ideally that part could also be autogenerated, but it would require to set some params in code-exporter (which part should be used for path etc and it's much more complicated). I think this should be good to go now and can be iterated on in future if anyone is interested.

Screenshot 2019-09-02 at 12 57 00

Let me know what you think of my changes @herecydev

herecydev added 2 commits Sep 2, 2019
@herecydev

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

Changes are good, I specifically avoided adding code after the query simply because you had to assume a lot, for example that you were query would return allData and that would have nodes etc...

I dug into the CodeExporter a little more and it provides a huge tree of the query, so my latest few commits are my attempt at producing a much deeper code snippet:

image

(and with comments)

image

A few things to note:

  • Checks for the presence of nodes on the query and suggests alternatives if it can't be found
  • Uses the node type to work out the variable name in forEach, i.e. allFoo will become foo
  • Checks for a field slug and uses that in the path
  • Checks for a field id and will add that to the context (as this is a good candidate)
  • Path is generated with the node type in mind, i.e. allFoo will become "./src/templates/foo.js"
  • Option to toggle comments
@herecydev

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

I suppose one other thing I would like to change is that although using export default ( ) => is nice and easy. I think it's a slippery slope and would always recommend explicitly naming your component otherwise you end up with a bunch of Unknown components in your React Tree where it has no information to work with.

What do you think to adding an option checkbox to toggle that behavior?

@pieh

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

Changes are good, I specifically avoided adding code after the query simply because you had to assume a lot, for example that you were query would return allData and that would have nodes etc...

Yeah, I do agree that all the snippet changes assumes much (tho, I tried to make it clear this is sample code), alternatively instead of adding code we could link to documentation for creating pages.

I dug into the CodeExporter a little more and it provides a huge tree of the query, so my latest few commits are my attempt at producing a much deeper code snippet:

Nice! huge tree of the query is called Abstract Syntax Tree (AST) ;) There is handy tool for inspecting them - https://astexplorer.net/#/gist/0810c322d06bf3d9ea7c9fb14d82fb9f/f19bcf0b1eb515f3535dd21d2f61a6724317a1dc (there all kinds of ASTs - so be sure to use GraphQL there if you end up using that)

Honestly I didn't want to this initially, because it complicates handler a lot. And it delays merging of this PR. Maybe we should drop createPages snippet from this PR, so we could merge initial implementation for page queries and both static queries variant and continue iterating on createPages in next PR as follow up PR?

I suppose one other thing I would like to change is that although using export default ( ) => is nice and easy. I think it's a slippery slope and would always recommend explicitly naming your component otherwise you end up with a bunch of Unknown components in your React Tree where it has no information to work with.

What do you think to adding an option checkbox to toggle that behavior?

I think we should avoid options when possible and try to generate snippets that would use as much of good practices as possible - so to me it makes perfect sense to use named Component by default.

@herecydev

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

Honestly I didn't want to this initially, because it complicates handler a lot

I fully expected this answer, there's a lot of value to be had but there's quite some complication as well.

Alright let me:

  1. drop the createPages snippet for now, we can revisit that later
  2. Add named components, there's really nothing to go on so I'll call it ComponentName unless you have a better suggestion

Can you confirm you're happy with the query names like useStaticQuery hook. Are they obvious how they differentiate and what they do?

@pieh

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

2. Add named components, there's really nothing to go on so I'll call it ComponentName unless you have a better suggestion

I don't have better suggestion. 👍 for it.

Can you confirm you're happy with the query names like useStaticQuery hook. Are they obvious how they differentiate and what they do?

I can't really come up with name that's objectively better, so 👍 for it (not a blocker)

@pieh

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

One note on ComponentName and useStaticQuery - we will likely hear from broader community of users when they start using them and potentially propose changes to those - will make sure to /cc you there ;)

@pieh
pieh approved these changes Sep 2, 2019
Copy link
Contributor

left a comment

Awesome! Let's release this!

@pieh pieh merged commit e723d76 into gatsbyjs:master Sep 2, 2019
20 checks passed
20 checks passed
Danger All good
Details
Peril All green. Yay.
Details
ci/circleci: bootstrap Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsby-image Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_path-prefix Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_gatsby_pipeline Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_long_term_caching Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: starters_validate Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node10 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node12 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node8 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_www Your tests passed on CircleCI!
Details
ci/circleci: windows_unit_tests Your tests passed on CircleCI!
Details
cypress: default-group 67 tests passed in 00:33
Details
unit_tests_windows Build #20190902.76 succeeded
Details
@gatsbot

This comment has been minimized.

Copy link

commented Sep 2, 2019

Holy buckets, @herecydev — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

@pieh

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

Published gatsby@2.15.3 / gatsby-graphiql-explorer@0.2.10

@pieh pieh referenced this pull request Sep 4, 2019
waltercruz added a commit to waltercruz/gatsby that referenced this pull request Sep 8, 2019
* Implement CodeExporter into graphiql

* Add some formatting

* Added a createPages snippet

* use same local storage key, as one that is being set

* invert condition - we want to check if it's explicitely set to true

* make createPages snippet richer, so full flow is accounted for

* Generate more detailed query

* Formatting

* Return name as default

* Return always from getFieldQuery

* Use named components
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.