-
Notifications
You must be signed in to change notification settings - Fork 254
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
Move __resolveReference
resolvers on to extensions
#1746
Conversation
The entity __resolveReference resolver is currently placed directly on to GraphQL*Type objects. This breaks the API, as the expectation is to put things like this on to the `extensions` field of objects contained within a GraphQLSchema. This moves the resolver off of the object and into the extensions where it belongs. Fixes: #1131 Fixes: ardatan/graphql-tools#2687 Fixes: ardatan/graphql-tools#2857
✅ Deploy Preview for apollo-federation-docs canceled.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
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.
🐐
Can we make this
|
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.
this looks good, and looks like it would still be straightforward to implement glasser's subgraph proposal, maybe moreso, so 👍
} | ||
}, | ||
}; | ||
} else if (fieldName.startsWith("__")) { | ||
(type as any)[fieldName.substring(2)] = fieldConfig; |
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.
@trevor-scheer is this basically just for resolveType
on abstract types and isTypeOf
on object types? wonder if we should just special case them (which lets us drop the type as any
).
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.
Also it looks like this entire directory (gateway-js/src/schema-helper
, not to be confused with subgraph-js/src/schema-helper
) is only used in tests? Can it be moved into gateway-js/src/__tests__
, or maybe you can just use the version in subgraph-js
directly from the tests?
@@ -0,0 +1,31 @@ | |||
import { GraphQLResolveInfo } from 'graphql'; | |||
|
|||
type GraphQLReferenceResolver<TContext> = ( |
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.
This file isn't imported from anywhere? Is it referenced in some config file that I'm missing? Why is it in gateway-js rather than subgraph-js?
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.
oh, i see, there's a second copy in subgraph-js. well, maybe my other suggestion will let us remove the duplication. and i guess as long as a file is included in the tsconfig'd project, it doesn't have to be imported for its declare module
to work?
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 do wonder though — if the file's not imported from anywhere, will the declare modules work across package? ie, this file generates a .d.ts that isn't sourced from index.d.ts, will it 'take effect' in the same way that it takes effect when you're actively building the package itself?
i mean maybe it doesn't matter since this is kinda an internal implementation detail of @apollo/subgraph
anyway.
Also including relevant work from #1658 to remove uses of __resolveObject
Includes relevant work from #1658 to remove uses of `__resolveObject`
# [2.0.2-alpha.0] - 2022-04-22 ## 🚀 Features - Improve fed1 schema support during composition [PR #1735](#1735) - Add gateway version to schema extensions [PR #1751](#1751) ## 🐛 Fixes - Improve merging of groups during `@require` handling in query planning [PR #1732](#1732) - Move `__resolveReference` resolvers on to `extensions` [PR #1746](#1746) - Honor directive imports when directive name is spec name [PR #1720](#1720) ## 🛠 Maintenance - Improved renovate bot auto-updates for 0.x packages [PR #1736](#1736) and [PR #1730](#1730) - Add missing `@apollo/federation-internals` dependency to gateway [PR #1721](#1721) - Migrate to `@apollo/utils` packages for `createSHA` and `isNodeLike` [PR #1765](#1765) ## 📚 Documentation - Roadmap updates! [PR #1717](#1717) - Clarify separation of concerns in the intro docs [PR #1753](#1753) - Update intro example for fed2 [PR #1741](#1741) - Improve error doc generation, add hints generation, add scrolling style to too-large error tables [PR #1740](#1740) - Update `supergraphSDL` to be a string when creating an `ApolloGateway` [PR #1744](#1744) - Federation subgraph library compatibility updates [PR #1718](#1744)
The location of `resolveReference` was updated in the `@apollo/subgraph` library to live on a type's `extensions` object. Reference: apollographql/federation#1746 apollographql/federation#1747
The entity __resolveReference resolver is currently placed directly
on to GraphQL*Type objects. This breaks the API, as the expectation
is to put things like this on to the
extensions
field of objectscontained within a GraphQLSchema.
This moves the resolver off of the object and into the extensions
where it belongs.
Fixes: #1131
Fixes: ardatan/graphql-tools#2687
Fixes: ardatan/graphql-tools#2857
cc @michael-watson @mandiwise