The current patterns in our GraphQL resolvers pass arguably inconsistent or ambiguous objects as the first (0th) positional argument meant to represent the "parent" per the GraphQL convention.
This ticket represents work to set standards on questions like:
what is the expected standard shape of that parent
where should the type of the parent be declared (shared? api? in the parent's file? in the child's file? all in a single types file? autogenerated types file from .graphql file?)
what is the expected kind/amount of information expected in there ("the whole kitchen sink" or "only the fields the child needs") - graphql suggests maybe the resolved parent (not the same thing as the database model from shared)
what should go in the "parent" vs. placed into context (info.context)
As an example, here is repositoryConfg's field resolvers whose "parent" type is the database model from shared and contains the entire database object passed from parent (repository) to child (repositoryConfig). Confusingly, the parent repository resolver's "parent" type here takes the same type as an input.
I'd argue that we place too much of a burden on developers to have deep knowledge of the codebase (and graphQL) before they can change one line of code in any resolver - would be nice to see how we can leverage types and/or better names to help reduce this and let anyone jumping in be able to draw a line from A to B without having implicit knowledge about chains of resolvers and where any ancestor has placed information into its return value ("parent") or Any-typed "context" that results in a "leap of faith".
Definition of done - proposed pattern + doc we can agree on to start implementing for net new models and have a plan for migrating the old ones to the new pattern, if needed. At a minimum, root out the "bad apples" (i.e., instances of "improper" patterns), so new developers can assume the code is written to graphql/our standard.
Declare types for the info.context. Currently Any and differs across queries so developers have to "guess" what's in there to some extent.
Consider splitting out types for the GraphQL resolved objects instead of using the Django database model types (e.g., those for owner, repository). This will add some more overhead of now requiring a mapping between the database model and the type used in the GraphQL layer, but it decouples the two, which may be nicer.
Examples / overviews in READMEs and such outlining how our GraphQL APIs work?
The current patterns in our GraphQL resolvers pass arguably inconsistent or ambiguous objects as the first (0th) positional argument meant to represent the "parent" per the GraphQL convention.
This ticket represents work to set standards on questions like:
shared
?api
? in the parent's file? in the child's file? all in a single types file? autogenerated types file from .graphql file?)shared
)info.context
)As an example, here is
repositoryConfg
's field resolvers whose "parent" type is the database model fromshared
and contains the entire database object passed from parent (repository) to child (repositoryConfig). Confusingly, the parentrepository
resolver's "parent" type here takes the same type as an input.I'd argue that we place too much of a burden on developers to have deep knowledge of the codebase (and graphQL) before they can change one line of code in any resolver - would be nice to see how we can leverage types and/or better names to help reduce this and let anyone jumping in be able to draw a line from A to B without having implicit knowledge about chains of resolvers and where any ancestor has placed information into its return value ("parent") or Any-typed "context" that results in a "leap of faith".
Definition of done - proposed pattern + doc we can agree on to start implementing for net new models and have a plan for migrating the old ones to the new pattern, if needed. At a minimum, root out the "bad apples" (i.e., instances of "improper" patterns), so new developers can assume the code is written to graphql/our standard.
Background / a proposed option - https://github.com/codecov/codecov-api/pull/806#discussion_r1772393338
Some possible low-hanging fruit:
info.context
. CurrentlyAny
and differs across queries so developers have to "guess" what's in there to some extent.owner
,repository
). This will add some more overhead of now requiring a mapping between the database model and the type used in the GraphQL layer, but it decouples the two, which may be nicer.