-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Feature: Adds Federation 2 Support #2115
Conversation
@frederikhors - Just a small question, as I can't seem to quite understand why it's erroring out on tests/using it. I have added a Link interface here: plugin/federation/fedruntime/runtime.go, however referencing within the federation file such as: builtins["link__Import"] = config.TypeMapEntry{
Model: config.StringList{"github.com/99designs/gqlgen/fedruntime.Link"},
} Still doesn't find the interface. |
@lleadbet we need to wait @StevenACoffman for this answer. |
```yml | ||
federation: | ||
filename: graph/generated/federation.go | ||
package: generated |
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.
Did you want to add a version: 2
key here?
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.
We can implicitly pull that from the schema, hence why I left it out; might be worthwhile, however.
@@ -11,3 +11,6 @@ type Service struct { | |||
type Entity interface { | |||
IsEntity() | |||
} | |||
|
|||
// Used for the Link directive | |||
type Link interface{} |
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'm idly wondering if the Link interface should have functions to introspectively answer:
- What version was imported (currently only "2.0" possible)
- What imports out of the list:
["@key","@shareable", "@provides", "@external", "@tag", "@extends", "@override", "@inaccessible"]
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.
Hmm- I think it might be useful in the future, but since the parser also validates at time of loading, you need to load all directives just in case- the current flow just ensures that the file contains that directive using Regex, which isn't the best solution either.
Not sure if you have any recs on how to handle the pre-parsing problem, though.
Applying my suggested fix removes one test failure and moves the second to:
This is because both the |
I modified that federation.gotpl to have this chunk to get the tests to succeed after applying the suggested fix in my other comment:
These are silly, but something better should go there or have them not be required. |
Agreed- was copying Apollo's material, but since those imports are just strings, decided to convert it into |
I have, however it still fails; would be curious on the fix here. |
You need to run |
Thanks! |
This PR adds in support for Apollo's Federation 2 standard, as requested per issue #2114.
This adds a new configuration value to set the federation version manually as well as a method to detect federation 2 schemas using a small regex string.
Largely, this doesn't change much about the generated code, but instead allows for
graphql-parser
to not error by adding the new Federation 2 directives before loading new schemas. Previously, this was panicing due to a missing directive.By default, this still uses Federation 1 unless detected by aforementioned regex or set by the user to avoid breaking changes.
I have: