Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: PX-5157 - Add Bulk Edit endpoint #4269
feat: PX-5157 - Add Bulk Edit endpoint #4269
Changes from 3 commits
91b3ac2
876478e
d308a36
57cd4b5
d272ea6
4e11439
26b01a5
76777ea
0af1226
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
GET
endpointartwork.ts
), and resolve itPOST
endpoint in prep for a mutationme
folder for an example)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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good 👍 (And I didn't know about this doc!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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