feat(federation): Use extensions instead of custom federation objects
#4313
Conversation
Composition in its current form modifies the graphql-js types in order to accommodate some additional metadata required during composition, validation, query planning, and execution. This is no longer required due to the addition of 'extensions', which this is intended to be used exactly for. This commit transitions our usages of these custom metadata objects into the extensions object where they belong.
trevor-scheer
force-pushed
the
trevor/federation-use-extensions
branch
from
41655a5 to
25a7c15
Compare
There was a problem hiding this comment.
LGTM w/ a couple notes within. Very exciting to flush out this older technique in favor of the functionality offered with graphql/graphql-js#2097! Thanks for your time working on this!
| declare module 'graphql/type/definition' { | ||
| interface GraphQLObjectType { | ||
| federation?: FederationType; | ||
| } | ||
|
|
||
| interface GraphQLEnumType { | ||
| federation?: FederationType; | ||
| } | ||
|
|
||
| interface GraphQLScalarType { | ||
| federation?: FederationType; | ||
| } | ||
|
|
||
| interface GraphQLInterfaceType { | ||
| federation?: FederationType; | ||
| } | ||
|
|
||
| interface GraphQLUnionType { | ||
| federation?: FederationType; | ||
| } | ||
|
|
||
| interface GraphQLInputObjectType { | ||
| federation?: FederationType; | ||
| } | ||
|
|
||
| interface GraphQLEnumValue { | ||
| federation?: FederationType; | ||
| } | ||
|
|
||
| interface GraphQLInputField { | ||
| federation?: FederationField; | ||
| } | ||
|
|
||
| interface GraphQLField<TSource, TContext> { | ||
| federation?: FederationField; | ||
| } | ||
| } | ||
|
|
||
| declare module 'graphql/type/directives' { | ||
| interface GraphQLDirective { | ||
| federation?: { | ||
| directiveDefinitions: { | ||
| [serviceName: string]: DirectiveDefinitionNode; | ||
| }; | ||
| }; | ||
| } | ||
| } | ||
|
|
| namedType.extensions = { | ||
| ...namedType.extensions, | ||
| federation: federationMetadata, | ||
| }; |
There was a problem hiding this comment.
In the event that the extensions property/object already exists (does it always, in newer graphql versions?), I can imagine that it might be beneficial to avoid creating new extensions objects and instead maintain referential equality to the existing property. For example, this could be relevant if we're federating existing schemas.
Might we consider merely adding the federation property to extensions? If it's indeed undefined until an implementor tries writing to it, we could default to creating it in that case?
There was a problem hiding this comment.
Note: we spoke offline about this and decided we'd move forward with this solution until it becomes a problem.
Sadly we can't mutate the extensions object itself as it's Readonly, so instead it must be recreated regardless of whether or not it already exists (if we're respecting the types, anyway).
For posterity, @IvanGoncharov mentioned that we can use transformSchema here. This may be the way forward for types, but it leaves us with the same solution for a field's extensions below. I'm also hesitant to use transformSchema here since it seems to require a rework of the existing approach. Rather than modify the types one at a time, I think we would need to collect a map of types and their new extensions during the for loop, followed by a transformSchema once we've mapped over the types.
| field.extensions = { | ||
| ...field.extensions, | ||
| federation: fieldFederationMetadata |
There was a problem hiding this comment.
Same concern for creating new extensions objects rather than re-cycling the old ones, when present.
| field.extensions = { | ||
| ...field.extensions, | ||
| federation: fieldFederationMetadata, |
| field.extensions = { | ||
| ...field.extensions, | ||
| federation: fieldFederationMetadata, |
| namedType.extensions = { | ||
| ...namedType.extensions, | ||
| federation: typeFederationMetadata, |
| directive.extensions = { | ||
| ...directive.extensions, | ||
| federation: directiveFederationMetadata, | ||
| } |
There was a problem hiding this comment.
@IvanGoncharov jut pointing out one more place we didn't address in our discussion: directive extensions!
…ts (apollographql/apollo-server#4313) Note: BREAKING CHANGE In order to guarantee the safe usage of extensions, we must now require `graphql@>14.5.0`. As such, we've narrowed the range in federation and gateway's `peerDependencies`. Composition in its current form modifies the graphql-js types in order to accommodate some additional metadata required during composition, validation, query planning, and execution. This is no longer required due to the addition of 'extensions', which this is intended to be used exactly for. This commit transitions our usages of these custom metadata objects into the extensions object where they belong. Apollo-Orig-Commit-AS: apollographql/apollo-server@39e678c
Composition in its current form modifies the
graphql-jstypes in order to accommodate some additional metadata required during composition, validation, query planning, and execution. This is no longer required due to the addition ofextensions, which is intended for exactly this use case.This PR transitions our usages of these custom metadata objects into the extensions object where they belong.
This work should unblock future work on #4310 and allow for some recommended cleanup.