Add targetType property to Mutation #639
Conversation
snaksa
force-pushed
the
add-mutation-target-type-property
branch
from
8e2b613 to
beff4de
Compare
snaksa
force-pushed
the
add-mutation-target-type-property
branch
from
beff4de to
3a7f24c
Compare
| @@ -673,7 +687,7 @@ private static function getGraphQLFieldsFromProviders(string $namespace, string | |||
| continue; | |||
| } | |||
|
|
|||
| $annotationTarget = 'Query' === $annotationName ? $annotation->targetType : null; | |||
| $annotationTarget = \in_array($annotationName, ['Query', 'Mutation']) ? $annotation->targetType : null; | |||
There was a problem hiding this comment.
With providers, $annotationName will always be either Query or Mutation, so this line is useless.
| @@ -289,8 +289,22 @@ private static function typeAnnotationToGQLConfiguration( | |||
| $isRootMutation = ($rootMutationType && $gqlName === $rootMutationType); | |||
| $currentValue = ($isRootQuery || $isRootMutation) ? \sprintf("service('%s')", self::formatNamespaceForExpression($reflectionEntity->getName())) : 'value'; | |||
|
|
|||
| $queryType = $isRootMutation ? 'Mutation' : 'Query'; | |||
There was a problem hiding this comment.
I think this part is incomplete and confusing. The Query and Mutation types are considered "root" regardless of the schema, so they should behave the same way (the $currentValue for example).
Also, if we can target whatever type with the @query, the @mutation should only target "root" Mutations.
I think we should check this and raise an exception if something other than a Mutation is targeted by a @Mutation.
Then we should change the isRootQuery & isRootMutation to check in all schema.
And the last part are the default Query / Mutation.
Now, we must decide what we want to target if there is no targetType provided. We used to target the root query and mutation of the default schema. Now, we can have multiple schemas and we are not sure that we have one with the name "default". So, my suggestion would be to target the one with name "default" if it exists, and if not the first one of the list of schemas.
And the last parameter of getGraphQLFieldsFromProviders should be rename $isDefault as it is used to know if the current type is the default one (the one targeted when no targetType is defined).
The tests should also be updated accordingly.
There was a problem hiding this comment.
Thank you for your feedback! I will try to implement the changes you mentioned tomorrow. I am new to using this bundle and I have some troubles understanding what do you mean by: the @mutation should only target "root" Mutations. Do you mean that there might be targetType set to something other than mutation type? And how can a @query target whatever type we choose? This confuses me a little and I guess everything will clear in my head once I understand this concept. Can you guide me to a piece of documentation that explains this and I might have missed?
There was a problem hiding this comment.
The idea of the @Provider is to be able to add query or mutation from various services.
In the case of Query, we can target the "root" Query (ie. The Query define at the key "query" in the schema configuration)...or...any other type (for example, I could target the type "Droid" like in the test fixtures) and the effect will be that I'll have a new queryable field on type Droid.
Let's say I want to provide a "getVictims" query on the Droid type, it would be queried like that for example:
query {
getDroid(id: 'HKB-3') { // Return a Droid
getVictims {
name
}
}
}
In the case of Mutation, the GraphQL specs says that mutations must be defined on the Mutation object (ie. The "root" mutation define at the key "mutation" in the schema config). So I couldn't target the Droid with my mutation. So the targetType should always be a root Mutation as Mutation are always "root".
Hope it'll help :)
|
Ping @snaksa ! |
|
Closing in favor of #710 |
Added targetType property to the Mutation type so that different schemas can be created and mutations can be attached to them.