-
Notifications
You must be signed in to change notification settings - Fork 397
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(@nestjs/graphql) fix federated schema generated with code first approach #1780
Conversation
…pproach Federated schema generated with the code first approach does not contain the directives. Closes nestjs#1597
@@ -3,7 +3,7 @@ import { loadPackage } from '@nestjs/common/utils/load-package.util'; | |||
import { isEmpty } from '@nestjs/common/utils/shared.utils'; | |||
import { gql } from 'apollo-server-core'; | |||
import * as chokidar from 'chokidar'; | |||
import { printSchema } from 'graphql'; | |||
import { printSchema } from '@apollo/federation'; |
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.
@apollo/federation
is an optional dependency so we'd have to make it required
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.
The GraphQLModule
is a wrapper around Apollo server, so it could make sense to have only the @apollo/federation
as a required dependency since it provides the Apollo Federation subgraph specifications (e.g. directives declaration).
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 think the problem here is that you can use @nestjs/graphql
in the scenario where graph is not federated and will never be, thus dependency is optional and not required for such case?
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 think that @kamilmysliwiec is referring to the fact that @apollo/federation
is an optional peer dependency:
Line 96 in 521d8e1
"optional": true |
'@apollo/federation', |
You can use the printSchema()
function of @apollo/federation
also in the scenario where the graph is not federated since Apollo has forked the code from graphql-js
(as reported in the source code) and patched the function to enable also the federation.
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.
The GraphQLModule is a wrapper around Apollo server, so it could make sense to have only the @apollo/federation as a required dependency since it provides the Apollo Federation subgraph specifications (e.g. directives declaration).
We can't make @apollo/federation
required as it's not needed for regular GraphQL apps. I'd say we should use the original printSchema
for non-federated graphs and the printSchema
from @apollo/federation
(lazy loaded) for Federation-based apps
@keadex I tried your solution and now in my generated federated subgraph I indeed get federated entities/types:
but the |
@ssukienn yes, it is an intended behavior. The federated subgraphs do not need the declaration of that directives since Apollo Server is already compatible, as reported in the Apollo Federation documentation: "This content is provided for developers adding federated subgraph support to a GraphQL server library, and for anyone curious about the inner workings of federation. It is not required if you're already using a subgraph-compatible library like Apollo Server." In fact in the schema first approach, the Nest.js documentation does not mention the need of that directives: https://docs.nestjs.com/graphql/federation#schema-first |
Not necessarily. To publish the subgraph in apollo studio to generate the composition of the supergraph, I need to pass the directives. In the supergraph contains the relationships |
@kamilmysliwiec @nestjs/graphql v10 is not fixing this behavior. Do you plan to fix it for the apollo driver or is there some other resolution? |
Let's track this here #2059 |
Here we were talking about "standard" directives like
These directives could be omitted if using a subgraph-compatible library like Apollo Server as reported in the Apollo Federation documentation. Actually I have different microservices subgraphs (https://github.com/keadex/keadex-einaudi) successfully published on Apollo Studio (you can also query: https://keadex.io/graphql) without these optional directives: |
Any update on this? |
Federated schema generated with the code first approach does not contain
the directives.
Closes #1597
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #1597
What is the new behavior?
The generated schema contains also the directives needed for the federation.
Does this PR introduce a breaking change?
Other information