gateway: remove use of internal Apollo Server function from LocalGraphQLDataSource #2007
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…hQLDataSource I'm not exactly sure what the purpose of LocalGraphQLDataSource is, other than that it's a convenient thing to use in this repository's test suite. It is exported from `@apollo/gateway` but does not appear to be documented. It currently uses a function stolen from the heart of Apollo Server. We are working on eliminating the dependency of Gateway on Apollo Server (apollographql/apollo-server#6719, apollographql/apollo-server#6057) so this dependency on something that's not even part of the supported API seems worth looking into. `enablePluginsForSchemaResolvers` does two entirely unrelated things. One is the thing that its name says: it tweaks the schema so that it's possible to instrument execution on a per-field basis; this is used to implement Apollo Server's `willResolveField` plugin hook. The other thing it does is entirely undocumented: it enables an obscure and undocumented `__resolveObject` feature which we are already planning to remove in Apollo Server 4. As mentioned, this class seems to really just be a test helper for this repository, and in fact, the tests did use `__resolveObject` until a few months ago in #1658. So one might guess that the point of this line is just to enable `__resolveObject` in tests and that it can be entirely removed. And in fact, ff45cb7 shows that there used to be a comment here saying exactly that! So this commit removes the use of an undocumented internal function to enable an even less documented feature. The tests pass, so that seems good. Perhaps one could object to this as not fully backwards compatible, because perhaps there are users out there who are using LocalGraphQLDataSource directly and... expect willResolveField plugins to work? But that's not really something you can make work without the rest of Apollo Server anyway. I guess maybe they could actually be expecting `__resolveObject` to work? But at that point we're talking about theoretical people trying to combine two undocumented features (`LocalGraphQLDataSource` and `__resolveObject`). If we discover that those people exist, we can encourage them to just add this line back into their own code themselves. Surely, people who enjoy the thrill of using two undocumented features will be even happier using three undocumented features!
✅ 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. |
benweatherman
approved these changes
Jul 22, 2022
glasser
deleted the
glasser/stop-using-an-undocumented-function-to-enable-an-undocumented-feature-inside-an-undocumented-class
branch
glasser
added a commit
that referenced
this pull request
Jul 22, 2022
…hQLDataSource (#2007) I'm not exactly sure what the purpose of LocalGraphQLDataSource is, other than that it's a convenient thing to use in this repository's test suite. It is exported from `@apollo/gateway` but does not appear to be documented. It currently uses a function stolen from the heart of Apollo Server. We are working on eliminating the dependency of Gateway on Apollo Server (apollographql/apollo-server#6719, apollographql/apollo-server#6057) so this dependency on something that's not even part of the supported API seems worth looking into. `enablePluginsForSchemaResolvers` does two entirely unrelated things. One is the thing that its name says: it tweaks the schema so that it's possible to instrument execution on a per-field basis; this is used to implement Apollo Server's `willResolveField` plugin hook. The other thing it does is entirely undocumented: it enables an obscure and undocumented `__resolveObject` feature which we are already planning to remove in Apollo Server 4. As mentioned, this class seems to really just be a test helper for this repository, and in fact, the tests did use `__resolveObject` until a few months ago in #1658. So one might guess that the point of this line is just to enable `__resolveObject` in tests and that it can be entirely removed. And in fact, ff45cb7 shows that there used to be a comment here saying exactly that! So this commit removes the use of an undocumented internal function to enable an even less documented feature inside an undocumented class. The tests pass, so that seems good. Perhaps one could object to this as not fully backwards compatible, because perhaps there are users out there who are using LocalGraphQLDataSource directly and... expect willResolveField plugins to work? But that's not really something you can make work without the rest of Apollo Server anyway. I guess maybe they could actually be expecting `__resolveObject` to work? But at that point we're talking about theoretical people trying to combine two undocumented features (`LocalGraphQLDataSource` and `__resolveObject`). If we discover that those people exist, we can encourage them to just add this line back into their own code themselves. Surely, people who enjoy the thrill of using two undocumented features will be even happier using three undocumented features!
glasser
added a commit
that referenced
this pull request
Jul 22, 2022
…hQLDataSource (#2007) (#2008) I'm not exactly sure what the purpose of LocalGraphQLDataSource is, other than that it's a convenient thing to use in this repository's test suite. It is exported from `@apollo/gateway` but does not appear to be documented. It currently uses a function stolen from the heart of Apollo Server. We are working on eliminating the dependency of Gateway on Apollo Server (apollographql/apollo-server#6719, apollographql/apollo-server#6057) so this dependency on something that's not even part of the supported API seems worth looking into. `enablePluginsForSchemaResolvers` does two entirely unrelated things. One is the thing that its name says: it tweaks the schema so that it's possible to instrument execution on a per-field basis; this is used to implement Apollo Server's `willResolveField` plugin hook. The other thing it does is entirely undocumented: it enables an obscure and undocumented `__resolveObject` feature which we are already planning to remove in Apollo Server 4. As mentioned, this class seems to really just be a test helper for this repository, and in fact, the tests did use `__resolveObject` until a few months ago in #1658. So one might guess that the point of this line is just to enable `__resolveObject` in tests and that it can be entirely removed. And in fact, ff45cb7 shows that there used to be a comment here saying exactly that! So this commit removes the use of an undocumented internal function to enable an even less documented feature inside an undocumented class. The tests pass, so that seems good. Perhaps one could object to this as not fully backwards compatible, because perhaps there are users out there who are using LocalGraphQLDataSource directly and... expect willResolveField plugins to work? But that's not really something you can make work without the rest of Apollo Server anyway. I guess maybe they could actually be expecting `__resolveObject` to work? But at that point we're talking about theoretical people trying to combine two undocumented features (`LocalGraphQLDataSource` and `__resolveObject`). If we discover that those people exist, we can encourage them to just add this line back into their own code themselves. Surely, people who enjoy the thrill of using two undocumented features will be even happier using three undocumented features!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm not exactly sure what the purpose of LocalGraphQLDataSource is,
other than that it's a convenient thing to use in this repository's test
suite. It is exported from
@apollo/gatewaybut does not appear to bedocumented.
It currently uses a function stolen from the heart of Apollo Server. We
are working on eliminating the dependency of Gateway on Apollo Server
(apollographql/apollo-server#6719,
apollographql/apollo-server#6057) so this
dependency on something that's not even part of the supported API seems
worth looking into.
enablePluginsForSchemaResolversdoes two entirely unrelated things.One is the thing that its name says: it tweaks the schema so that it's
possible to instrument execution on a per-field basis; this is used to
implement Apollo Server's
willResolveFieldplugin hook.The other thing it does is entirely undocumented: it enables an obscure
and undocumented
__resolveObjectfeature which we are already planningto remove in Apollo Server 4.
As mentioned, this class seems to really just be a test helper for this
repository, and in fact, the tests did use
__resolveObjectuntil a fewmonths ago in #1658. So one might guess that the point of this line is
just to enable
__resolveObjectin tests and that it can be entirelyremoved.
And in fact, ff45cb7 shows that there used to be a comment here
saying exactly that!
So this commit removes the use of an undocumented internal function to
enable an even less documented feature inside an undocumented class.
The tests pass, so that seems good.
Perhaps one could object to this as not fully backwards compatible,
because perhaps there are users out there who are using
LocalGraphQLDataSource directly and... expect willResolveField plugins
to work? But that's not really something you can make work without the
rest of Apollo Server anyway. I guess maybe they could actually be
expecting
__resolveObjectto work? But at that point we're talkingabout theoretical people trying to combine two undocumented features
(
LocalGraphQLDataSourceand__resolveObject). If we discover thatthose people exist, we can encourage them to just add this line back
into their own code themselves. Surely, people who enjoy the thrill of
using two undocumented features will be even happier using three
undocumented features!