Build Federated Schemas From Existing Schema

[j]

Hello!

So I'm implementing federation in type-graphql and saw that there's some improvements that I feel will make 3rd party libraries not using typeDefs & resolver maps for creating schemas.

This adds the ability to pass in an existing GraphQLSchema object and does the following if the argument passed to buildFederatedSchema is an instance of GraphQLSchema:

1. Iterates over the schema's type's to get a map of the "resolveReference" functions
   1a. First checks type's resolveReference property
   1b. Checks if type has a field named __resolveReference (also deletes this field after saving the function)
2. Creates the federation specific object types
3. Adds the resolvers found in the schema using addResolversToSchema

Everything else stays the same. I ended up creating a utility function to all of the tests to automatically test a wrapped version of the federated schema as well as included one manual one.

This will make federation support super easy for projects like type-graphql instead of doing printSchema hacks, converting to typeDefs and plucking resolvers out to then be re-converted to SDL and schema, etc. We can then not have federation as a dependency but instead require @apollo/federation on our own app and expose the federated server.

References:
MichalLytek/type-graphql#369

Example Usage in type-graphql: MichalLytek/type-graphql@9bbed68

[j]

@JacksonKearl Thoughts on this?

[j]

I implemented directives into type-graphql and it made it super easy to build federated graphs this way. Here's a full working example: MichalLytek/type-graphql@9bbed68

[JacksonKearl]

Hey! Super exciting to see this coming together, it will be great to be able to federate services from even more backends. I left a few initial comments, and pinging @jbaxleyiii for a more complete review.

[j]

I can cleanup the tests. I wrapped them all just to quickly reuse the tests in place then another wrapped call which is the schema instance.

[j]

@JacksonKearl thoughts?

[j]

@JacksonKearl I cleaned the implementation up. It should be more clear on what's going on. Let me know what you think. /cc @jbaxleyiii

We feature-locked our API until this is implemented so we can move to federated services, so I'm willing to fix changes in near real time. And others using the type-graphql library will benefit from this soon! :)

Also, when graphql-js implements extensions configuration, this can be cleaned up even more. Right now I have to extract federation specific resolvers from field configuration and type properties to later remap them. When extensions comes along, then resolveReference can be invoked directly from the extensions property of the type and type.toConfig() and technically addResolversToSchema won't need to be called on already constructed schemas... it would just work! :)

[j]

@abernix thoughts on this PR? We really want to start moving our existing servers to federation without having to use the resolvers + SDL method. Right now, you almost HAVE to convert schema to SDL + resolvers which is pretty ugly :(

[jbaxleyiii]

@j we are going to do another pass of reviews first of next week. A few of us are out of office right now which is why there has been some delay on this review! Thank you so much for working on this, very excited to get it merged in

[j]

@jbaxleyiii Sounds good! This is going to help adoption a ton! Thanks! 🏄

[ldiego08]

Ah, this is amazing!

[j]

@ldiego08 thanks! Federation support in type-graphql will be 100% compatible when this is merged and MichalLytek/type-graphql#369 (which is pretty much done) 👍

[j]

@JacksonKearl it's saying there's changes requested still but I don't see any. I thought I resolved them all. :\

[martijnwalraven]

I don't think this needs to be an array of GraphQLResolverMap.

[martijnwalraven]

Alternatively, we could merge resolvers from all modules here, so we can simplify transformFederatedSchema.

[martijnwalraven]

This could be simplified if we always pass in a merged GraphQLResolverMap.

[j]

@martijnwalraven I was trying to keep the logic the same as what is currently happening where we're iterating over the modules and calling addResolversToSchema when module.resolvers is defined. How do you propose I merge resolvers without adding too much logic into this method. Do I iterate over resolvers, check if it's a scalar type and over-ride, otherwise do a "merge"?

[martijnwalraven]

Note that we may soon be able to remove these workarounds and switch to using extensions instead, perhaps as soon as the end of this month: graphql/graphql-js#1527 (comment)

[pierreis]

Thank you! ♥️

[ldiego08]

💪

[joeldevelops]

+1 on this. There is a ticket at work that I'm blocked on while waiting for this PR. Trying to migrate our backend codebase to the future 💻

[eole1712]

How can I help u guys ?
Really excited to make this work, so we can start to implement federation support on type-graphql !

[snys98]

It's been about a month... I wish I could help but what is really blocking this?

[j]

@JacksonKearl I keep getting these pings of people wanting this. What are your thoughts to move forward? Extensions is out in graphql-js, so this can be cleaned up.

[trevor-scheer]

@j apologies for the delay on this. We've had some discussions about upgrading the graphql version within federation and have decided that we'd like to go forward with it as we'll be using the extensions API ourselves in the near future. Are you still interested in simplifying the implementation here (and upgrading graphql peerDep to ^14.5.0)? If so, we'd love to move forward with this PR!

[itsezc]

Are there currently any updates with regards to this?

[pierreis]

Is there any way we can help this PR moving forward?

[LexSwed]

Just to bring attention to this PR
@tjwallace do you need any help with that?

[Superd22]

@j 's fork seems to have disappeared from his GitHub, i've taken the liberty of taking his branch and rebasing on master on my own fork.

We've been using this PR internally for the last few months and would love to see it makes its way to upstream. @trevor-scheer @jbaxleyiii is the rebase enough or do you guys need the graphql-js bump & extensions rewrite to go through with it ?

[j]

Oops, @Superd22 I went on a cleaning spree. I for some reason had >200 forks. Thanks. I haven't had time to implement this as we decided this was too new and aren't implementing it at this time.

[bjanjua-inmoment]

@Superd22 / @j Great work on this. Has there been any progress with getting this into the main Apollo Server build? @Superd22 Would you have time to make a new PR for this?

[stephtr]

Would it be possible to merge this PR as it is (apart from resolving the conflicts) and work on the extensions rewrite later when someone has time?

[nivas-everest]

@j Any update on this PR. I have a similar requirement in my project. When can we expect this to be production-ready?

[j]

@nivas-everest We since then decided to not use federation until it's proven. It shouldn't be hard to build atop this.

[stephtr]

@nivas-everest We since then decided to not use federation until it's proven. It shouldn't be hard to build atop this.

If one doesn't have any experience with this codebase, at least for me it's unfortunately difficult to follow. How exactly do the extensions fit in?

[dijbi]

Fixed with an upcoming update ?

[abernix]

This was an unintentional closing. Please stand-by and see #4304 for more details.

(While I could put the branch back in place and re-open this right now, I'd prefer to let GitHub Support diagnose what might have gone wrong and try to explain why only a portion of pull-requests were re-targeted and the rest closed, in order to make sure the (somewhat recently introduced) pull-request re-targeting feature is working properly for other GitHub users.)

[abernix]

This is one of 5 PRs that I was unable to re-open after the process described #4304. The 4 others I couldn't re-open were because they had already been re-opened, and this was because the source branch had been deleted.

To preserve this, I've re-opened this as a new PR, myself, from the same ref (refs/pull/3013/head) in #4310. Please see/follow that PR for details, I'm going to lock this one to hopefully migrate the conversation there going forward. So sorry for the noise!