-
Notifications
You must be signed in to change notification settings - Fork 257
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
Adds @tag
definition automatically in buildSubgraphSchema
#1600
Conversation
This patch makes `@tag` behave like any other federation directive in `subgraph-js`/`buildSubgraphSchema`, that is, its definition is automatically added to the schema. But because we want to be "backward compatible", this patch also ensure that federation directive definitions are not added by `buildSugraphSchema` if the input document already defined a directive with that name. Do note that while `subrgaph-js` has no validation that such user definition of a federation directive is "correct", this is ok because composition does do such validation and will reject an invalid definition. A consequence of this patch is that the `@tag` definition is not outputed by `printSubgraphSchema` (like for other federation directives), but it's very doubtful any external user would have relied on this behaviour in the first place (and it's very easy to adapt in the off chance someone does).
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. |
buildSubgraphSchema
@tag
definition automatically in buildSubgraphSchema
]; | ||
|
||
export function isFederationDirective(directive: GraphQLDirective): boolean { | ||
return federationDirectives.some(({ name }) => name === directive.name); | ||
} | ||
|
||
export const otherKnownDirectives = [TagDirective]; |
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.
Quick note for reviewers: an option could have been to keep that notion of "known but not officially federation" directives but have it empty for now. But if anything, we're trying to move away from hard-coded directives and toward @link
-defined directives, so I'm almost certain this would never be used again, so removed it (in fact, #1554 is probably going to change all of this before GA anyway).
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.
Agree that it's better to delete code that is unused rather than having it hand around!
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.
🐐
`); | ||
}; | ||
|
||
it('adds it for fed1 schema', async () => { |
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.
nitpick/question We're saying that the tag directive is not added for fed1, right?
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.
No, it's added. Though there is a little bit of subtlety here with what "fed1" means. Here we mean "fed1 schema that use subgraph-js 2.0+". People continuing to use subgraph-js 0.x won't have it added automatically however (it's a different branch altogether and I don't suggest we backport there).
We could have it not added for "fed1 schema handled by fed2 code" of course, but I'm essentially suggesting we don't bother because it's more code to have it happen (since currently subgraph-js
don't really have logic distinguishing fed1 vs fed2 schema, it's pretty basic) and "fed1 schema handled by fed2 code" already have additional flexibility compared to "fed1 schema handled by fed1 code" so why not 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.
So I understand the individual pieces, but the reasons why this PR are necessary are a little fuzzy to me. Maybe worth spending a few minutes during the meeting today to give an overview.
@@ -67,7 +86,7 @@ export function buildSubgraphSchema( | |||
modules, | |||
new GraphQLSchema({ | |||
query: undefined, | |||
directives: [...specifiedDirectives, ...federationDirectives], | |||
directives: [...specifiedDirectives, ...missingFederationDirectives(modules)], |
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.
So if I understand correctly, we are going through the AST to figure out which federation directives are not present and then passing them in a list? Can you explain why it isn't the federation directives that are present that we want?
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.
When we visit the AST, we only look at directive definitions, not application. And what we're adding is the directives definition that don't already have a definition in the user provided schema.
We're not looking at applications at all. Which does mean we may be adding @provides
definition even though it's never used by that particular schema, and we could well avoid it, but buildSubgraphSchema
has always done that, adding all federation directive definition regardless of what is being used (which is harmless), and so keeping the code change focused.
The problem we're solving with this is that we want the @tag
definition to be added automatically, like all other federation directive definitions, but the difference is that we historically have asked fed1 user to provide the definition themselves. So to be backward compatible, we just need to ensure we don't try to auto-add definitions that are already user defined, which wasn't something buildSubgraphSchema
was previously bothering with.
@@ -30,6 +32,23 @@ type LegacySchemaModule = { | |||
|
|||
export { GraphQLSchemaModule }; | |||
|
|||
function missingFederationDirectives(modules: GraphQLSchemaModule[]): GraphQLDirective[] { | |||
// Copying the array as we're going to modify it. | |||
const missingDirectives = federationDirectives.concat(); |
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.
May not matter too much, but it would probably be better to use a set to avoid the O(n) lookup in a loop.
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.
Well, it's one those rare case where the array size is not dynamic, we know exactly how many elements the array, it's a very small number and we kind of know it's never going to grow to a large number (relatively). So complexity argument kind of don't apply, and I'd be surprise if the array version is not as fast if not faster (but I agree it doesn't matter either way in practice).
]; | ||
|
||
export function isFederationDirective(directive: GraphQLDirective): boolean { | ||
return federationDirectives.some(({ name }) => name === directive.name); | ||
} | ||
|
||
export const otherKnownDirectives = [TagDirective]; |
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.
Agree that it's better to delete code that is unused rather than having it hand around!
@@ -518,6 +518,50 @@ extend type User @key(fields: "email") { | |||
} | |||
`); | |||
}); | |||
|
|||
describe('@tag directive', () => { |
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.
indent issue
`); | ||
}; | ||
|
||
it('adds it for fed1 schema', async () => { |
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.
consider using it.each
when you have multiple cases like this.
This patch makes
@tag
behave like any other federation directive insubgraph-js
/buildSubgraphSchema
, that is, its definition isautomatically added to the schema.
But because we want to be "backward compatible", this patch also
ensure that federation directive definitions are not added by
buildSugraphSchema
if the input document already defined a directivewith that name. Do note that while
subrgaph-js
has no validationthat such user definition of a federation directive is "correct",
this is ok because composition does do such validation and will
reject an invalid definition.
A consequence of this patch is that the
@tag
definition is notoutputed by
printSubgraphSchema
(like for other federationdirectives), but it's very doubtful any external user would have
relied on this behaviour in the first place (and it's very easy
to adapt in the off chance someone does).