Propose updates to GraphQL extensibility doc #404
Conversation
DrewML
requested review from
kokoc,
akaplya,
nrkapoor,
paliarush,
cpartica,
danielrenaud,
avattam06 and
prabhuram93
| password: String!, | ||
| multi_factor_auth_token: String! | ||
| ): GenerateCustomerTokenOutput! | ||
| If a query or mutation accepts (or will likely accept) > 1 argument, an Input Object should be used instead, and given the argument name `input`. |
There was a problem hiding this comment.
I would say we must always use input because we never know how Magento is/will be extended.
There was a problem hiding this comment.
I think it's very likely you're right here. I have a tendency to try and leave a little wiggle room 😄
Do you want to make any exception for the case of looking something up by ID? I think in the case of a field like Query.productByID (as an example) it might be nice to just take the ID, and if folks want to add arguments to a unary field, they just have to introduce a new query instead.
If you think this isn's worth it, happy to go forward with your suggested change.
There was a problem hiding this comment.
Inability to extend is the reason why I don't like byId methods. I would use input everywhere. Simper rule - less bugs.
There was a problem hiding this comment.
Inability to extend is the reason why I don't like byId methods
Does every query need to be infinitely extensible? Can you describe a scenario where a Query.fooByID field would need to have its arguments extended, and where that would make more sense than adding a new query? Both a new argument and a new field would require changes for a headless UI to consume already, so it's not less work for the client.
I think, for single entity lookups (not queries for collections) there aren't many scenarios where it would make sense to take more than an ID as input. Most other arguments you'd add to that method would likely change the behavior as well (breaking change).
Please correct me if I'm wrong, but it seems like a lot of the reasons to add more arguments to a field might be a sign that, for simple cases like this, the field is trying to do too much.
There's also the question of naming - anytime we use a field like Query.product instead of Query.productByID, we're using a more generic name that's likely to be overloaded later for either uses, leading to things like deprecations and _v2 fields. Explicit names and granular fields in this case (imo) kind of force the query to be a bit more focused.
Simper rule - less bugs.
Isn't it the opposite in this case? Adding an argument to an existing field adds branching to a resolver, and a developer needs to take great care to make sure that logic does not impact any existing queries.
There was a problem hiding this comment.
I don't have strong opinion against byId, but would really like to have input mandatory.
There was a problem hiding this comment.
We can do that 👍 Will make changes
Co-authored-by: Alex Paliarush <[email protected]>
|
@melnikovi @nrkapoor can y'all give this a review when you find time? |
melnikovi
added
the
needs update
Rendered Document
I noticed the extensibility document was missing some content while I was updating it with proposed solution from #400. This PR contains changes to address #400 and some additional guidelines we (mostly) already follow.
Feedback appreciated :)
Reviewers list populated with folks that commonly contribute to our schema.