-
Notifications
You must be signed in to change notification settings - Fork 254
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
fix(federation): Print @tag and @inaccessible directives conditionally #859
fix(federation): Print @tag and @inaccessible directives conditionally #859
Conversation
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.
Other than some comments about naming, this looks good to me!
/** | ||
* A set of directive names that have been used at least once | ||
*/ | ||
type AppliedDirectiveUsages = Set<string>; |
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.
This is not a blocker, but the term 'applied directive usages' is a bit weird because both 'applied directives' and 'directive usages' are meant to differentiate between directive definitions and their use.
@@ -21,7 +21,7 @@ import { | |||
DirectiveNode, | |||
} from 'graphql'; | |||
import { transformSchema } from 'apollo-graphql'; | |||
import apolloTypeSystemDirectives from '../directives'; | |||
import apolloTypeSystemDirectives, { appliedDirectives, federationDirectives } from '../directives'; |
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.
I had missed this before, but appliedDirectives
doesn't seem like the right name here, because these exports are directive definitions (as opposed to directive applications, which is what we find on AST nodes). Maybe something like otherKnownDirectives
could work here?
Noting the recommendations are strictly naming/internally facing related and I'm hoping to get in on a Rover release, I'm going to merge now and follow up with naming fixes today. |
@trevor-scheer agree with your decision to expedite and follow-up with the suggestions that @martijnwalraven offered. thanks! |
Only print the directive definitions and
@core
declarations for the@tag
and@inaccessible
directives in the supergraph SDL if there are usages of them found in the subgraph schemas.