Add federation support #455
Conversation
|
I could still use some help for one more test case I'd like to cover. Marcus provided a string token to register a // graphql-gateway.module.ts
@Module({})
export class GraphQLGatewayModule implements OnModuleInit {
private apolloServer: ApolloServerBase;
constructor(
@Optional()
private readonly httpAdapterHost: HttpAdapterHost,
@Optional()
@Inject(GRAPHQL_GATEWAY_BUILD_SERVICE) // <-- This thing here
private readonly buildService: GatewayBuildService,
@Inject(GRAPHQL_GATEWAY_MODULE_OPTIONS)
private readonly options: GatewayModuleOptions,
) {}I figured something like this would work, but sadly no luck class BuildService extends RemoteGraphQLDataSource {
willSendRequest() {
console.log('RECEIVED!!');
}
}
@Module({
imports: [
GraphQLGatewayModule.forRoot(/* config */),
],
providers: [{
provide: GRAPHQL_GATEWAY_BUILD_SERVICE,
useClass: BuildService,
}],
})
export class AppModule {}edit: @marcus-sa you'll probably know best how to fix this one :) |
|
Btw you should upgrade the Apollo dependencies as there's a breaking change in the gateway class options. The way I initially wanted to implement the BuildService was an injectable that works like an async options factory basically (hopefully this gives you a clue) HINT: The reason why the provider is unavailable in the module context, is because:
|
|
I rebased your branch on master where Apollo is already updated but I forgot to check the changelog. Will do that tomorrow, thanks for the heads up! Figured if was something like that, tried injecting it into a dynamic module using the |
tuxmachine
force-pushed
the
feat/federation
branch
from
0f56138
to
f094b13
Compare
@kamilmysliwiec Ready for a review 👍 |
tuxmachine
force-pushed
the
feat/federation
branch
from
9f85b82
to
e734b53
Compare
|
Docs added nestjs/docs.nestjs.com#802 |
|
Does this PR also provide a way to share the context across services? |
|
Yes it does, but maybe the docs need to be clearer om that. https://deploy-preview-802--docs-nestjs.netlify.com/graphql/federation#build-service |
|
If there's anything more that I can do regarding this PR, please let me know :) happy to spend some more time on it. |
|
@tuxmachine Have you tested Custom scalars and Enum internal values features from ApolloServer with this ? I tried federation using @marcus-sa branch you started working from I believe and passing scalars/enums in the resolvers argument when importing GraphQLFederationModule did not work. For example with the GraphQLJSON Scalar from graphql-type-json package: It seems the JSON scalar is basically ignored, and any kind of input can go through. |
|
@ZFLloyd nope, I never got to that point. |
|
@ZFLloyd Thanks for highlighting a missing piece. My real-world experience with both nest and graphql are limited, so the feedback is really valuable. I'll see what I can do this week. |
|
@tuxmachine @marcus-sa However, when working with Apollo Graph Manager, this line fire error // lib/graphql-gateway.module.ts:L107
const { schema, executor } = await gateway.load();Error Logso I changed the code as below to make it works. // lib/graphql-gateway.module.ts:L102-L108
const gateway = new ApolloGateway({
...gatewayOpts,
buildService,
});
// const { schema, executor } = await gateway.load(); // Skip the load at this point.
this.registerGqlServer({ ...serverOpts, gateway, subscriptions: false });By doing so, both the production (use Apollo Graph Manager) and development (use References: |
|
@yeuem1vannam Thanks for testing with Graph Manager! Your fix works with the existing tests so I added it to the PR. |
|
@ZFLloyd I believe I've fixed it. Could you verify? |
|
One question left: will this implementation work with the |
|
@renepardon nope, because |
|
@marcus-sa so in this case i should not focus on the code first approach rather then writing the schema by myself? Good to know before I start adding to much decorators to my classes :) |
|
Should we create a new issue which stays open until MichalLytek/type-graphql#77 is released? |
|
It looks like So yeah, this PR fixes the schema-first approach. There needs to be extra work done for the code-first approach but it appears to no longer be blocking EDIT: The support for directives is merged to master but still unreleased. |
|
@kamilmysliwiec I'm assuming you'd like to wait with this feature until it can support the code-first approach? Other than that, anything else you'd like to see? |
|
@kamilmysliwiec do you have any ETA for this feature to be released? |
Replaced the Pick with it's inversion which allows more Apollo Server options to pass through.
Blocked until support for Directives lands in type-graphql[1] 1. MichalLytek/type-graphql#351
tuxmachine
force-pushed
the
feat/federation
branch
from
1076b1a
to
c8ba590
Compare
|
@tuxmachine, is there anything important fixed in your last batch of commits? Thanks |
|
@XBeg9 Not really. Rebased on master to keep everything up to date. Two minor things:
|
|
Thanks for your work on this @tuxmachine @kamilmysliwiec Do you have any updates on review/merge? Is there anything us invested in this issue can do to help? |
|
Is there anything holding this PR up? |
If it's just conflicts on the package.json, maybe I can help? |
Nope it's just a large PR, takes time to review and maintainers are very busy. |
|
Thank you! Published as 6.6.0 :) |
|
@tuxmachine, @marcus-sa Thanks for yours contributions. @kamilmysliwiec Thanks for accepting, I believe you must be a very busy person. With this, NestJS becomes even more versatile, and its built-in modules are very helpful, making developers' lives a little easier. |
|
@kamilmysliwiec Thanks for the review and merge! Feel free to ping me if issues come up |
|
How are we looking on the code-first implementation? Does it work? If not, what's the current blocker? |
|
@endurance I already have a service up and running on Amazon ECS using a code-first implementation! So it does semm to work! 🎉 |
|
@endurance remember to use For more information: https:/MichalLytek/type-graphql/blob/master/examples/apollo-federation/inventory/product.ts Only use the class Product and import all decorators from @nestjs/graphql Regards! |
|
@tuxmachine would you like to create a PR with an update to the docs (code first way)? It works exactly the same as in e2e tests you wrote ( |
|
@kamilmysliwiec Yeah no problem. I'll try to write the docs for code-first federation somewhere this month. |
|
Thank you @tuxmachine! |
|
@lookapanda is there any chance you could share some basic code until the docs have been written? I'm attempting to migrate to a code-first federated server, but |
|
@joemckie In rough lines:
Look at the test files for some examples using code-first federation |
|
@tuxmachine thanks! Will give that a try 😄 |
|
@tuxmachine pinging in you here hoping you'll get a notification 😅 what's the best way to reach out to you? or better, can you message me on twitter/linkedin/via email ([email protected])? |
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:
#299
#288
What is the new behavior?
Support for Apollo Federation
Does this PR introduce a breaking change?
Other information
This is a follow-up PR to #301 by @marcus-sa . Many thanks to his original implementation, I want to use it and had some spare time, so I implemented the unit tests and will start the documentation tomorrow. Don't know the correct etiquette for this sort of situation, opening up a new PR seemed easiest.