feat: PX-5157 - Add Bulk Edit endpoint #4269
Conversation
| 1. Start your local server with `yarn start` | ||
| 1. With your favourite graphql app, you should be able to hit the local server to refresh the schema. In insomnia, you can click on the "schema" button in the GraphQL panel to manually refresh this, if you don't have auto-refresh turned on (also found in this list) | ||
| 1. You should be able to hit your local metaphysics, which will hit staging gravity by default. | ||
| 1. Work through the errors, making sure to read the message in its entirety. Also don't hesitate to add it to the errors section below if useful. |
There was a problem hiding this comment.
This point is also a bit confusing because the above describes setup, and now suddenly there are errors. Maybe another point can be added in between that says. "Now, develop your feature!" (or something).
Also it's not so clear where the dev should be seeing errors. Are they server errors, typescript errors, graphql schema errors? If this ambiguity forces too much explaination here, best to just omit this point completely.
There was a problem hiding this comment.
I based this more or less off of my own experience, so it can definitely be improved for someone else.
With this section I was trying to prepare the developer to see errors (you are right in that this should be specified for what errors and where) but also to encourage further additions to the documentation if someone comes across an error that could be documented to help someone else get setup quicker.
I don't mind if we remove this point from these steps, however I believe there definitely needs to be a section on debugging and error expectations
|
|
||
| ### Tips on writing and debugging locally | ||
|
|
||
| - GraphQL is strongly typed and needs to know the exact inputs and outputs of the endpoint as it mediates between the clients and the REST endpoint. This is why you need to set up a union type which instructs GraphQL on what both responses will look like. This allows GraphQL tools to know if the request you are making (in Insomnia, for example) is valid, even before you send the request. |
There was a problem hiding this comment.
Going from setup to writing mutation-handling Union types is a very steep curve!
As we want this document to be instructive (rather than intimidating) its best to keep this all very incremental versus skipping ahead. Meaning:
- Add an example that shows how to add a new loader pointing at a Gravity
GETendpoint - Show how to add a new field to a section within the schema (say,
artwork.ts), and resolve it - Show how to add a new
POSTendpoint in prep for a mutation - Show had to add a new mutation (in the simplest way possible; no fancy error handling is necessary yet; see all of the mutations in the
mefolder for an example) - THEN show how to handle errors in a way that is more sophisticated.
But even so, this thingOrError pattern isn't necessarily something we should advocate for up front in a getting started doc. In many ways it's overly complicated for everyday mutations and should be used more on a need to know basis.
(Worth noting that we have a full blog post on this here: https://artsy.github.io/blog/2018/10/19/where-art-thou-my-error/)
There was a problem hiding this comment.
Thanks for the article link, I will have to check it out and also link it in this doc.
I guess I'm simultaneously both the best and worst person to make this doc. The best person because I know almost nothing about GraphQL and can give feedback on whether something is too complicated or not, and also the worst person because I know nothing about GraphQL and so I shouldn't be teaching others about things I don't really understand.
The thingOrError I thought was standard and came from the example PR I was basing my changes on. Happy to learn the easier way on how to do this!
| partnerArtworksOrError: { | ||
| type: UpdatePartnerArtworksMutationType, | ||
| resolve: (result) => { | ||
| console.log(result) |
There was a problem hiding this comment.
For devs who use VSCode, one can boot MP in the VSCode console and have automatic debugger support by adding a breakpoint. Maybe we should lean into vs console.log-style debugging.
There was a problem hiding this comment.
What are you thoughts about leaving the console log in because it is very basic, but also including a link to the debugger approach?
There was a problem hiding this comment.
That sounds good 👍 (And I didn't know about this doc!)
|
|
||
| ### Potential Errors | ||
|
|
||
| This below error points to the fact that the `UpdatePartnerArtworksMutationType` was not resolving for either the `SuccessType` or the `FailureType`. GraphQL is very strict and it needs to categorize everything into types and fails when it can't. The solution to this was to ensure that the `SuccessType` was correctly defined. This means that the `isTypeOf` check needs to look for a success criteria in the response. In this scenario, it was incorrect and needed to be updated to `isTypeOf: (data) => data.success`. |
There was a problem hiding this comment.
Similar to the comment above, this error is a bit too specific and tied to product work for a getting started doc! If we're trying to onboard devs we need to start with the simplest use-case first and then incrementally increase complexity. Starting here is too scary and specific 👻
There was a problem hiding this comment.
This is based on my personal experience and I wanted to have a potential errors section where we can list generic errors that might help people find the root cause of their own errors, without relying on someone with more GraphQL knowledge. I didn't pay attention to all the errors that I experienced before I decided to write this doc so unfortunately I only have this one. I found it super helpful after understanding the error on how the code determines whether something is a success or an error. I think it is useful here (something along these lines, of the response not being a success or an error) however happy to move it somewhere else talking generally about the job of each function
There was a problem hiding this comment.
LGTM and fine to merge/iterate. I think the PartnerArtworks name should be improved, and I'm a bit confused as to the underlying bulk endpoint (seems like a successful mutation might still include errors (like for individual artwork errors?)) so it's a bit of an odd duck but totally fine to iterate on!
There was a problem hiding this comment.
@Mitch-Henson - the docs are a good start! I would suggest pulling them out of this PR and into something separate which we can develop outside of feature work and get some more reviewers.
(But yeah, looking here I agree that our docs are woefully inadequate for introducing a new dev to GraphQL. I think if we start slow and then build we could get a simple guide going for folks that would reduce the learning curve substantially.)
|
Thanks for the feedback so far! As per @damassi suggestion, I've moved the docs into its own PR here: #4273 so that it doesn't block feature work. @mzikherman I've addressed most of your concerns except I'm still struggling with removing the clientMutationId from the input and also making sure that the |
…eck. Removed unused internal fields
|
Looks great, thanks for bearing with all the comments! Could use a spec but otherwise pretty perfect. |
|
Whoops, you can ignore the re-review request @damassi. It hadn't loaded correctly and looked like your previous review was blocking, even though you dismissed it. I will merge this in now to unblock some Volt work, but will add some tests and tag you in that follow-up PR @mzikherman |
Solves PX-5157
This PR adds the Bulk Edit endpoint from Gravity into Metaphysics so that it can be consumed by coming changes in Volt.
As part of these changes, I've introduced some more beginner/backend friendly documentation on adding a REST endpoint into Metaphysics. 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.