feat: Beginner Friendly GraphQL Doc #4273
Conversation
Mitch-Henson
requested review from
damassi,
mzikherman,
leamotta and
nspinazz89
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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_gravityand 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):
- Start small. Begin with setup, as you're currently doing, and then move into simple
GETs - Document how to add a new
RESTendpoint tolib/loaders/loaders_without_authentication/gravity.ts - Show how to access (and resolve) that data by adding a new field to a slice of the schema -- say,
artworkorartist.ts - 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 - Show how to write a simple mutation, following this example from the
src/schema/mefolder. 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). - 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 👍
… Removing some complicated errors, adding more relevant links for further reading
|
|
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 |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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 --
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.