Fix propagation of @tag to supergraph and related issues #1592
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
The @tag directive was properly merged into supergraphs due to some recent changes to the merging of directives and this patch fixes this. Proper testsing for @tag is also added to avoid regressions. Those additional tests also highligted that the code wasn't declaring the @tag directive as repeatable, limiting its uses, and this is fixed as well. Lastly, this patch also raises the question of applying @tag on an external field, and by extension of any merged directives on external fields. Before this patch, such directives were _not_ merged but silently so. This patch changes the behavior to reject such directives upfront as it feels better than ignoring them silently.
pcmanus
force-pushed
the
tag-propagation
branch
from
fc4dc95
to
d6d5951
Compare
| @@ -62,6 +62,7 @@ export const shareableDirectiveSpec = createDirectiveSpecification({ | |||
| export const tagDirectiveSpec = createDirectiveSpecification({ | |||
| name:'tag', | |||
| locations: tagLocations, | |||
| repeatable: true, | |||
There was a problem hiding this comment.
It's probably ok for this case, but is there a situation where we'd want a directive to be repeatable on the subgraph spec, but not the supergraph?
There was a problem hiding this comment.
Good question, not sure. I feel ideally a directive behaviour/semantic should be defined in isolation and not depend on where it's used, and whether it make sense to repeat it or not on an element is more based on what the directive means.
That is, I could see wanting to preserve all the subgraph-level information for a directive in a supergraph, but it almost feels like this supergraph directive should be a different one at that point (because it's semantic would be slightly different).
Anyway, I could be wrong and we may need a way to deal with such things at some point :). But we don't have a clean way to do it currently.
That said, just for completeness, composition does have a bit of flexibility built-in as far as directive definition goes, and a subgraph can define @tag without repeatable locally to ensure no usage in the subgraph is repeated, and composition will essentially ignore that local definition. But don't think this is what you were asking about.
| @@ -130,7 +130,7 @@ describe('printSubgraphSchema', () => { | |||
|
|
|||
| type User @key(fields: \\"id\\") @tag(name: \\"from-reviews\\") { | |||
| id: ID! | |||
| username: String @external @tag(name: \\"on-external\\") | |||
| username: String @external | |||
There was a problem hiding this comment.
Do we want another test to make sure that the old case errors?
Edit: Never mind, I see the test in compose.test.ts
The @tag directive was properly merged into supergraphs due to some
recent changes to the merging of directives and this patch fixes this.
Proper testsing for @tag is also added to avoid regressions. Those
additional tests also highligted that the code wasn't declaring the
@tag directive as repeatable, limiting its uses, and this is fixed
as well.
Lastly, this patch also raises the question of applying @tag on
an external field, and by extension of any merged directives on
external fields. Before this patch, such directives were not merged
but silently so. This patch changes the behavior to reject such
directives upfront as it feels better than ignoring them silently.