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: Beginner Friendly GraphQL Doc #4273

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Mitch-Henson
Copy link
Contributor

This PR attempts to add some more backend / beginner friendly documentation on how to add a GraphQL endpoint for an existing Gravity endpoint. This was previously added in a previous PR but is being moved to its own PR to allow for more discussion without blocking any feature work.

As a first time user of GraphQL I definitely struggled even with the most basic of operations and hopefully this will help other future devs with their first PR.

Copy link
Contributor

@kajatiger kajatiger left a comment

Choose a reason for hiding this comment

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

That's a nice addition to the existing docs, thanks for adding that!
What helped me was also the docs that you can find in some projects for example the ones in exchange for the preparation of the metaphysics change.

araujobarret
araujobarret previously approved these changes Aug 2, 2022
Copy link
Contributor

@araujobarret araujobarret left a comment

Choose a reason for hiding this comment

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

Loved the initiative, all look good from my side! 🎸

I would maybe add a section to say briefly about data loaders and provide a link to the other doc https://github.com/artsy/metaphysics/blob/main/docs/dataloaders.md, people working in newer features might bump in a need of a data loader that was never implemented before

Copy link
Member

@damassi damassi left a comment

Choose a reason for hiding this comment

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

I agree, this is a really good first step!

Similar to @araujobarret's comment, I think before this is merged we should step through some of the basics just so a totally new GraphQL dev doesn't arrive at this document and get confused. Meaning: There's the setup steps (good!), the code running steps (also good), and then one suddenly lands upon more advanced topics like handling errors via complex patterns (union types) that aren't quite idiomatic to the wider GraphQL community (and are scary, even for those who have been using GraphQL for a while).

And so I would suggest two possible paths:

  • We could rename this file to something along the lines of error_handling_patterns_in_gravity and strip out some of the setup steps or move them into the main README; or
  • Develop this doc further to be more comprehensive and lead into error handling patterns as a secondary concern

I'm hesitant to approve this as it is just because GraphQL is such an intimidating technology at first, and so being overly cautious about baby steps is crucial. The current document's name is also somewhat conflicting because there are no actual steps here that talk about adding a new endpoint, as one would expect changes to lib/loaders/loaders_with/without_authentication/gravity.ts file, and then the corresponding wire-up in a schema file (and so on).

So, if wanting to go with the latter option, here are some suggestions (which would also be a good exercise to work through docs-wise just because you're new to it):

  1. Start small. Begin with setup, as you're currently doing, and then move into simple GETs
  2. Document how to add a new REST endpoint to lib/loaders/loaders_without_authentication/gravity.ts
  3. Show how to access (and resolve) that data by adding a new field to a slice of the schema -- say, artwork or artist.ts
  4. With the absolute must-have basics now out of the way (which shows how to add a new Gravity Endpoint to MP, per the document's aim), now show how to do the next most crucial operation, a POST
  5. Show how to write a simple mutation, following this example from the src/schema/me folder. This format linked here handles most everything that we need, from handling errors from gravity, to typing inputs with TypeScript. It's how we should introduce new devs to mutations, and is a gentle approach and the one that developers will most commonly use going forward (as opposed to the union-type example herein).
  6. Finally, once adding a new endpoint and writing a basic mutation is covered, we can then handle advanced concepts like our union-type mutations, but not before explaining the reason why we would want to use this pattern. (If we can't adequately explain why a dev should use this approach, it should be omitted for the sake of teaching.)

I don't mean to blow up the scope of this document 🙏 but I also want to really encourage us to think about what's most relevant to a new developer's needs and to introduce them slowly to the concepts. Totally understand that a lot of the things in this document reflect your first experience in MP and could be valuable for others; however, practically-speaking it is a rare day when a new GraphQL dev needs to dive into complex topics like unions and start writing mutations, where alternatively, it's very common for someone to need to add a Gravity GET endpoint, and then access it from a new field that resolves said data via the newly-created dataLoader.

Getting the prerequisites simply expressed will yield the greatest impact from an organizational perspective, so it's worth spending the time doing it right -- especially because we currently don't have a comprehensive doc that goes over this adequately.

If you feel inspired to flesh this out further, totally down to pair on this and fill in any gaps 👍

@damassi damassi requested a review from dzucconi August 2, 2022 17:09
… Removing some complicated errors, adding more relevant links for further reading
@artsy-peril
Copy link
Contributor

artsy-peril bot commented Aug 4, 2022


Warnings
⚠️ The V2 schema in this PR has breaking changes with Force. Remember to update the Force schema if necessary.

Type 'UpdatePartnerArtworksMutationInput' was removed
Type 'UpdatePartnerArtworksMutationPayload' was removed
Type 'UpdatePartnerArtworksMutationType' was removed
Type 'UpdatePartnerArtworksMutationFailure' was removed
Type 'UpdatePartnerArtworksMutationSuccess' was removed
Type 'UpdatePartnerArtworksMutationSuccessDetails' was removed
Type 'PartnerArtworksBulkEditErrors' was removed
Field 'matchArtist' was removed from object type 'Query'
Field 'matchArtist' was removed from object type 'Viewer'
Field 'updatePartnerArtworks' was removed from object type 'Mutation'
Input field 'shippingTotalCents' was removed from input object type 'CommerceSellerAcceptProvisionalOfferInput'
Input field 'shippingTotalCents' was removed from input object type 'CommerceSellerCounterOfferInput'

Generated by 🚫 dangerJS against 2c392e8

@Mitch-Henson
Copy link
Contributor Author

Thanks for everyone's feedback! I've made considerable updates, though it is not completely finished.

I would love to take you up on your pairing offer @damassi, but as I'm in Berlin, I think it is almost impossible. I've taken your very thorough feedback to heart and have moved a lot of things around. Let me know if the current Adding a GET request section is more of what you had in mind!

The Adding a POST request and Writing a mutation sections are still both empty, and I will have to pair with someone to make sure that I don't spread misinformation in this doc!


<!-- This should render the code inline -->

https://github.com/artsy/metaphysics/blob/79a387cd837d48485bf6a0aa00e8999ad323ff23/src/lib/loaders/loaders_with_authentication/gravity.ts#L39
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest using the loaders_without_authentication/gravity file instead, just so new devs can practice without needing to be authenticated (meaning, without needing to add an additional X-Access-Token header into their GraphiQL app)


https://github.com/artsy/metaphysics/blob/79a387cd837d48485bf6a0aa00e8999ad323ff23/src/lib/apis/gravity.ts#L14-L43

If you have worked with Gravity before, this code should be recogniseable. It takes the previously given partial path and makes a call to Gravity for this endpoint. This Gravity loader is then [matched](https://github.com/artsy/metaphysics/blob/79a387cd837d48485bf6a0aa00e8999ad323ff23/src/schema/v2/Match.ts#L33) with its specific type, the Artwork type. This shows GraphQL the structure and type all of the potential fields that can come back from the Gravity endpoint.
Copy link
Member

Choose a reason for hiding this comment

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

We should probably end this paragraph here:

If you have worked with Gravity before, this code should be recogniseable. It takes the previously given partial path and makes a call to Gravity for this endpoint.

Short and sweet. The reasoning is, in all my years working in MP i've never once opened this Match file!

Copy link
Member

@damassi damassi left a comment

Choose a reason for hiding this comment

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

These updates are looking absolutely fantastic @Mitch-Henson 💯. Nice and clear and covers all the basics well. Just a couple minor comments.

I would argue that once some of the placeholders are filled in, even without the mutation section this is mergable 👍 Having the basic mutation overview would be a (helpful) bonus, though. Short of that, maybe a section called Writing Mutations with a few bullet points with links pointing to good examples would be sufficient. First bullet point could be the example I referenced up above, and the second could be the more sophisticated one with the union type (along with a link to our blog post).

Thanks again for working through these updates --

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.

4 participants