-
Notifications
You must be signed in to change notification settings - Fork 242
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 merging of renamed @tag directive ... #1637
Conversation
} | ||
|
||
const MERGED_FEDERATION_DIRECTIVES: MergedDirectiveInfo[] = [ |
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.
If someone wonders why this patch is kind of over-generic despite only have 1 merged directive, that's because @inaccessible
will be add to this list shortly.
👷 Deploy request for apollo-federation-docs pending review.Visit the deploys page to approve it
|
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. |
... but requires that it is renamed consistently in all subgraphs.
e75dd98
to
61b452c
Compare
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 question a bit whether this needs to be an error in the long term or if we can just do something smart, but I guess it could lead to confusion when examinging the supergraph if naming is inconsistent. Otherwise LGTM.
And for the record, I agree with that questioning. This PR is meant to be a stop-gap until we figure out the full rules we want for merging core directives. |
... but requires that it is renamed consistently in all subgraphs.