-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Make it possible to use AS4 with Apollo Gateway #6719
Milestone
Comments
Strategy:
|
glasser
added a commit
to apollographql/federation
that referenced
this issue
Jul 22, 2022
…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!
glasser
added a commit
to apollographql/federation
that referenced
this issue
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
to apollographql/federation
that referenced
this issue
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
to apollographql/federation
that referenced
this issue
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!
7 tasks
glasser
added a commit
to apollographql/federation
that referenced
this issue
Aug 2, 2022
The point of apollo-server-caching is to provide an abstraction over multiple cache backends. Gateway is not using that abstraction; it's just using one particular implementation, which is a wrapper around an old version of lru-cache. As part of apollographql/apollo-server#6057 and apollographql/apollo-server#6719 we want to remove dependencies on Apollo Server from Apollo Gateway. Technically we don't really need to remove this dependency (since apollo-server-caching doesn't depend on anything else in AS) but apollo-server-caching won't be updated any more (in fact, even in AS3 it has already been replaced by `@apollo/utils.keyvaluecache`), so let's do it. While we're at it, we make a few other improvements: - Ever since #440, the queryPlanStore field is always set, so we can remove some conditionals around it. - Instead of using the old lru-cache@6 wrapped by the apollo-server-caching package, we use the newer lru-cache@7 (which improves the algorithm internally and changes the names of methods a bit). - The get and set methods on InMemoryLRUCache were only async because they implement the abstract KeyValueCache interface: the implementations didn't actually do anything async. So we no longer need to await them or include a giant comment about how we're not awaiting them.
glasser
added a commit
to apollographql/federation
that referenced
this issue
Aug 2, 2022
The point of apollo-server-caching is to provide an abstraction over multiple cache backends. Gateway is not using that abstraction; it's just using one particular implementation, which is a wrapper around an old version of lru-cache. As part of apollographql/apollo-server#6057 and apollographql/apollo-server#6719 we want to remove dependencies on Apollo Server from Apollo Gateway. Technically we don't really need to remove this dependency (since apollo-server-caching doesn't depend on anything else in AS) but apollo-server-caching won't be updated any more (in fact, even in AS3 it has already been replaced by `@apollo/utils.keyvaluecache`), so let's do it. While we're at it, we make a few other improvements: - Ever since #440, the queryPlanStore field is always set, so we can remove some conditionals around it. - Instead of using the old lru-cache@6 wrapped by the apollo-server-caching package, we use the newer lru-cache@7 (which improves the algorithm internally and changes the names of methods a bit). - The get and set methods on InMemoryLRUCache were only async because they implement the abstract KeyValueCache interface: the implementations didn't actually do anything async. So we no longer need to await them or include a giant comment about how we're not awaiting them.
glasser
added a commit
to apollographql/federation
that referenced
this issue
Aug 2, 2022
This is part of apollographql/apollo-server#6057 (which is itself part of apollographql/apollo-server#6719). We are trying to break the dependency of Gateway on Server so that (among other things) it is easier to have a single version of Gateway that works with both the current AS3 and the upcoming AS4. In AS4, we are removing the ApolloError class and its subclasses. Instead, we will just use GraphQLError directly. See: https://www.apollographql.com/docs/apollo-server/v4/migration#apolloerror https://www.apollographql.com/docs/apollo-server/v4/migration#built-in-error-classes apollographql/apollo-server#6355 apollographql/apollo-server#6705 This commit changes RemoteGraphQLDataSource to throw GraphQLError instead of ApolloError. The `code` extension will still be the same. (The `name` field of the thrown Error will no longer be eg `AuthenticationError`, though; this does not affect the error as serialized in GraphQL.) This is technically slightly backwards-incompatible (eg, the method errorFromResponse is public and now returns GraphQLError instead of the tighter ApolloError) but this doesn't seem likely to affect many users. We can adjust based on feedback if necessary.
glasser
added a commit
to apollographql/federation
that referenced
this issue
Aug 2, 2022
…2028) This is part of apollographql/apollo-server#6057 (which is itself part of apollographql/apollo-server#6719). We are trying to break the dependency of Gateway on Server so that (among other things) it is easier to have a single version of Gateway that works with both the current AS3 and the upcoming AS4. In AS4, we are removing the ApolloError class and its subclasses. Instead, we will just use GraphQLError directly. See: https://www.apollographql.com/docs/apollo-server/v4/migration#apolloerror https://www.apollographql.com/docs/apollo-server/v4/migration#built-in-error-classes apollographql/apollo-server#6355 apollographql/apollo-server#6705 This commit changes RemoteGraphQLDataSource to throw GraphQLError instead of ApolloError. The `code` extension will still be the same. (The `name` field of the thrown Error will no longer be eg `AuthenticationError`, though; this does not affect the error as serialized in GraphQL.) This is technically slightly backwards-incompatible (eg, the method errorFromResponse is public and now returns GraphQLError instead of the tighter ApolloError) but this doesn't seem likely to affect many users. We can adjust based on feedback if necessary.
glasser
added a commit
to apollographql/federation
that referenced
this issue
Aug 2, 2022
The point of apollo-server-caching is to provide an abstraction over multiple cache backends. Gateway is not using that abstraction; it's just using one particular implementation, which is a wrapper around an old version of lru-cache. As part of apollographql/apollo-server#6057 and apollographql/apollo-server#6719 we want to remove dependencies on Apollo Server from Apollo Gateway. Technically we don't really need to remove this dependency (since apollo-server-caching doesn't depend on anything else in AS) but apollo-server-caching won't be updated any more (in fact, even in AS3 it has already been replaced by `@apollo/utils.keyvaluecache`), so let's do it. While we're at it, we make a few other improvements: - Ever since #440, the queryPlanStore field is always set, so we can remove some conditionals around it. - Instead of using the old lru-cache@6 wrapped by the apollo-server-caching package, we use the newer lru-cache@7 (which improves the algorithm internally and changes the names of methods a bit). - The get and set methods on InMemoryLRUCache were only async because they implement the abstract KeyValueCache interface: the implementations didn't actually do anything async. So we no longer need to await them or include a giant comment about how we're not awaiting them.
glasser
added a commit
to apollographql/federation
that referenced
this issue
Aug 2, 2022
…2028) This is part of apollographql/apollo-server#6057 (which is itself part of apollographql/apollo-server#6719). We are trying to break the dependency of Gateway on Server so that (among other things) it is easier to have a single version of Gateway that works with both the current AS3 and the upcoming AS4. In AS4, we are removing the ApolloError class and its subclasses. Instead, we will just use GraphQLError directly. See: https://www.apollographql.com/docs/apollo-server/v4/migration#apolloerror https://www.apollographql.com/docs/apollo-server/v4/migration#built-in-error-classes apollographql/apollo-server#6355 apollographql/apollo-server#6705 This commit changes RemoteGraphQLDataSource to throw GraphQLError instead of ApolloError. The `code` extension will still be the same. (The `name` field of the thrown Error will no longer be eg `AuthenticationError`, though; this does not affect the error as serialized in GraphQL.) This is technically slightly backwards-incompatible (eg, the method errorFromResponse is public and now returns GraphQLError instead of the tighter ApolloError) but this doesn't seem likely to affect many users. We can adjust based on feedback if necessary.
glasser
added a commit
to apollographql/federation
that referenced
this issue
Aug 2, 2022
* gateway: remove dependency on apollo-server-caching (#2029) The point of apollo-server-caching is to provide an abstraction over multiple cache backends. Gateway is not using that abstraction; it's just using one particular implementation, which is a wrapper around an old version of lru-cache. As part of apollographql/apollo-server#6057 and apollographql/apollo-server#6719 we want to remove dependencies on Apollo Server from Apollo Gateway. Technically we don't really need to remove this dependency (since apollo-server-caching doesn't depend on anything else in AS) but apollo-server-caching won't be updated any more (in fact, even in AS3 it has already been replaced by `@apollo/utils.keyvaluecache`), so let's do it. While we're at it, we make a few other improvements: - Ever since #440, the queryPlanStore field is always set, so we can remove some conditionals around it. - Instead of using the old lru-cache@6 wrapped by the apollo-server-caching package, we use the newer lru-cache@7 (which improves the algorithm internally and changes the names of methods a bit). - The get and set methods on InMemoryLRUCache were only async because they implement the abstract KeyValueCache interface: the implementations didn't actually do anything async. So we no longer need to await them or include a giant comment about how we're not awaiting them. * gateway RemoteGraphQLDataSource: throw GraphQLError, not ApolloError (#2028) This is part of apollographql/apollo-server#6057 (which is itself part of apollographql/apollo-server#6719). We are trying to break the dependency of Gateway on Server so that (among other things) it is easier to have a single version of Gateway that works with both the current AS3 and the upcoming AS4. In AS4, we are removing the ApolloError class and its subclasses. Instead, we will just use GraphQLError directly. See: https://www.apollographql.com/docs/apollo-server/v4/migration#apolloerror https://www.apollographql.com/docs/apollo-server/v4/migration#built-in-error-classes apollographql/apollo-server#6355 apollographql/apollo-server#6705 This commit changes RemoteGraphQLDataSource to throw GraphQLError instead of ApolloError. The `code` extension will still be the same. (The `name` field of the thrown Error will no longer be eg `AuthenticationError`, though; this does not affect the error as serialized in GraphQL.) This is technically slightly backwards-incompatible (eg, the method errorFromResponse is public and now returns GraphQLError instead of the tighter ApolloError) but this doesn't seem likely to affect many users. We can adjust based on feedback if necessary. * Adjust #2028 for [email protected] compatibility
Note: also remove the compile-time dependency of |
glasser
added a commit
that referenced
this issue
Aug 5, 2022
This PR does a few things: (a) Moves `error.extensions.exception.stacktrace` to `error.extensions.stacktrace`. (b) Removes the rest of `error.extensions.exception`. This contained enumerable properties of the original error, which could lead to surprising information leakage. (c) Documents that change and provides a `formatError` hook in the migration guide to maintain AS3 behavior. (d) Provide an `unwrapResolverError` function in `@apollo/server/errors` to help get back the original error in the resolver error case, which is a helpful part of the documented `formatError` recommendations (since the old error formatting put stuff from that unwrapped error on the `exception` extension). (e) Gets rid of the `declare module` which made `error.extensions.exception.code`/`error.extensions.exception.stacktrace` have a non-unknown value. Note that this declaration (added in 3.5.0) was actually inaccurate as `code` really goes directly on `extensions` rather than on `exception`. We could have instead preserved the declaration and adapted it to the new location of `stacktrace` and the correct location of `code`. - Pro: `declare module` is a bit scary and doesn't always merge well if you have more than one of them (eg, if you end up with both AS3 and AS4 in your TypeScript build: AS3 had a different `declare module` where `code` was nested under `exception`). - Con: End users who catch our errors can't get "correct" typing for `error.extensions.code` or `error.extensions.stacktrace`. - Pro: That's only "correct" if you assume all GraphQLErrors use extensions compatible with the definition; it might be better to export a helper function that takes a `GraphQLError` and returns the code/exception if it looks like it has the right shape. And nobody seems to have even noticed that the declaration has been wrong for almost a year, so how much value is it providing? (f) Renames the (new in v4) constructor option includeStackTracesInErrorResponses to includeStacktraceInErrorResponses, to match the extension name. This is to some degree part of #6719 because we're concerned about the effect of the `declare module` on the Gateway transition.
glasser
added a commit
that referenced
this issue
Aug 5, 2022
This PR does a few things: (a) Moves `error.extensions.exception.stacktrace` to `error.extensions.stacktrace`. (b) Removes the rest of `error.extensions.exception`. This contained enumerable properties of the original error, which could lead to surprising information leakage. (c) Documents that change and provides a `formatError` hook in the migration guide to maintain AS3 behavior. (d) Provide an `unwrapResolverError` function in `@apollo/server/errors` to help get back the original error in the resolver error case, which is a helpful part of the documented `formatError` recommendations (since the old error formatting put stuff from that unwrapped error on the `exception` extension). (e) Gets rid of the `declare module` which made `error.extensions.exception.code`/`error.extensions.exception.stacktrace` have a non-unknown value. Note that this declaration (added in 3.5.0) was actually inaccurate as `code` really goes directly on `extensions` rather than on `exception`. We could have instead preserved the declaration and adapted it to the new location of `stacktrace` and the correct location of `code`. - Pro: `declare module` is a bit scary and doesn't always merge well if you have more than one of them (eg, if you end up with both AS3 and AS4 in your TypeScript build: AS3 had a different `declare module` where `code` was nested under `exception`). - Con: End users who catch our errors can't get "correct" typing for `error.extensions.code` or `error.extensions.stacktrace`. - Pro: That's only "correct" if you assume all GraphQLErrors use extensions compatible with the definition; it might be better to export a helper function that takes a `GraphQLError` and returns the code/exception if it looks like it has the right shape. And nobody seems to have even noticed that the declaration has been wrong for almost a year, so how much value is it providing? (f) Renames the (new in v4) constructor option includeStackTracesInErrorResponses to includeStacktraceInErrorResponses, to match the extension name. This is to some degree part of #6719 because we're concerned about the effect of the `declare module` on the Gateway transition.
glasser
added a commit
that referenced
this issue
Aug 5, 2022
This PR does a few things: (a) Moves `error.extensions.exception.stacktrace` to `error.extensions.stacktrace`. (b) Removes the rest of `error.extensions.exception`. This contained enumerable properties of the original error, which could lead to surprising information leakage. (c) Documents that change and provides a `formatError` hook in the migration guide to maintain AS3 behavior. (d) Provide an `unwrapResolverError` function in `@apollo/server/errors` to help get back the original error in the resolver error case, which is a helpful part of the documented `formatError` recommendations (since the old error formatting put stuff from that unwrapped error on the `exception` extension). (e) Gets rid of the `declare module` which made `error.extensions.exception.code`/`error.extensions.exception.stacktrace` have a non-unknown value. Note that this declaration (added in 3.5.0) was actually inaccurate as `code` really goes directly on `extensions` rather than on `exception`. We could have instead preserved the declaration and adapted it to the new location of `stacktrace` and the correct location of `code`. - Pro: `declare module` is a bit scary and doesn't always merge well if you have more than one of them (eg, if you end up with both AS3 and AS4 in your TypeScript build: AS3 had a different `declare module` where `code` was nested under `exception`). - Con: End users who catch our errors can't get "correct" typing for `error.extensions.code` or `error.extensions.stacktrace`. - Pro: That's only "correct" if you assume all GraphQLErrors use extensions compatible with the definition; it might be better to export a helper function that takes a `GraphQLError` and returns the code/exception if it looks like it has the right shape. And nobody seems to have even noticed that the declaration has been wrong for almost a year, so how much value is it providing? (f) Renames the (new in v4) constructor option includeStackTracesInErrorResponses to includeStacktraceInErrorResponses, to match the extension name. This is to some degree part of #6719 because we're concerned about the effect of the `declare module` on the Gateway transition.
glasser
added a commit
that referenced
this issue
Aug 5, 2022
This PR does a few things: (a) Moves `error.extensions.exception.stacktrace` to `error.extensions.stacktrace`. (b) Removes the rest of `error.extensions.exception`. This contained enumerable properties of the original error, which could lead to surprising information leakage. (c) Documents that change and provides a `formatError` hook in the migration guide to maintain AS3 behavior. (d) Provide an `unwrapResolverError` function in `@apollo/server/errors` to help get back the original error in the resolver error case, which is a helpful part of the documented `formatError` recommendations (since the old error formatting put stuff from that unwrapped error on the `exception` extension). (e) Gets rid of the `declare module` which made `error.extensions.exception.code`/`error.extensions.exception.stacktrace` have a non-unknown value. Note that this declaration (added in 3.5.0) was actually inaccurate as `code` really goes directly on `extensions` rather than on `exception`. We could have instead preserved the declaration and adapted it to the new location of `stacktrace` and the correct location of `code`. - Pro: `declare module` is a bit scary and doesn't always merge well if you have more than one of them (eg, if you end up with both AS3 and AS4 in your TypeScript build: AS3 had a different `declare module` where `code` was nested under `exception`). - Con: End users who catch our errors can't get "correct" typing for `error.extensions.code` or `error.extensions.stacktrace`. - Pro: That's only "correct" if you assume all GraphQLErrors use extensions compatible with the definition; it might be better to export a helper function that takes a `GraphQLError` and returns the code/exception if it looks like it has the right shape. And nobody seems to have even noticed that the declaration has been wrong for almost a year, so how much value is it providing? (f) Renames the (new in v4) constructor option includeStackTracesInErrorResponses to includeStacktraceInErrorResponses, to match the extension name. This is to some degree part of #6719 because we're concerned about the effect of the `declare module` on the Gateway transition.
glasser
added a commit
that referenced
this issue
Aug 5, 2022
This PR does a few things: (a) Moves `error.extensions.exception.stacktrace` to `error.extensions.stacktrace`. (b) Removes the rest of `error.extensions.exception`. This contained enumerable properties of the original error, which could lead to surprising information leakage. (c) Documents that change and provides a `formatError` hook in the migration guide to maintain AS3 behavior. (d) Provide an `unwrapResolverError` function in `@apollo/server/errors` to help get back the original error in the resolver error case, which is a helpful part of the documented `formatError` recommendations (since the old error formatting put stuff from that unwrapped error on the `exception` extension). (e) Gets rid of the `declare module` which made `error.extensions.exception.code`/`error.extensions.exception.stacktrace` have a non-unknown value. Note that this declaration (added in 3.5.0) was actually inaccurate as `code` really goes directly on `extensions` rather than on `exception`. We could have instead preserved the declaration and adapted it to the new location of `stacktrace` and the correct location of `code`. - Pro: `declare module` is a bit scary and doesn't always merge well if you have more than one of them (eg, if you end up with both AS3 and AS4 in your TypeScript build: AS3 had a different `declare module` where `code` was nested under `exception`). - Con: End users who catch our errors can't get "correct" typing for `error.extensions.code` or `error.extensions.stacktrace`. - Pro: That's only "correct" if you assume all GraphQLErrors use extensions compatible with the definition; it might be better to export a helper function that takes a `GraphQLError` and returns the code/exception if it looks like it has the right shape. And nobody seems to have even noticed that the declaration has been wrong for almost a year, so how much value is it providing? (f) Renames the (new in v4) constructor option includeStackTracesInErrorResponses to includeStacktraceInErrorResponses, to match the extension name. This is to some degree part of #6719 because we're concerned about the effect of the `declare module` on the Gateway transition.
glasser
added a commit
that referenced
this issue
Aug 5, 2022
This PR does a few things: (a) Moves `error.extensions.exception.stacktrace` to `error.extensions.stacktrace`. (b) Removes the rest of `error.extensions.exception`. This contained enumerable properties of the original error, which could lead to surprising information leakage. (c) Documents that change and provides a `formatError` hook in the migration guide to maintain AS3 behavior. (d) Provide an `unwrapResolverError` function in `@apollo/server/errors` to help get back the original error in the resolver error case, which is a helpful part of the documented `formatError` recommendations (since the old error formatting put stuff from that unwrapped error on the `exception` extension). (e) Gets rid of the `declare module` which made `error.extensions.exception.code`/`error.extensions.exception.stacktrace` have a non-unknown value. Note that this declaration (added in 3.5.0) was actually inaccurate as `code` really goes directly on `extensions` rather than on `exception`. We could have instead preserved the declaration and adapted it to the new location of `stacktrace` and the correct location of `code`. - Pro: `declare module` is a bit scary and doesn't always merge well if you have more than one of them (eg, if you end up with both AS3 and AS4 in your TypeScript build: AS3 had a different `declare module` where `code` was nested under `exception`). - Con: End users who catch our errors can't get "correct" typing for `error.extensions.code` or `error.extensions.stacktrace`. - Pro: That's only "correct" if you assume all GraphQLErrors use extensions compatible with the definition; it might be better to export a helper function that takes a `GraphQLError` and returns the code/exception if it looks like it has the right shape. And nobody seems to have even noticed that the declaration has been wrong for almost a year, so how much value is it providing? (f) Renames the (new in v4) constructor option includeStackTracesInErrorResponses to includeStacktraceInErrorResponses, to match the extension name. (g) Removes a test around error handling a particular `yup` style of errors that is probably not relevant any more. This is to some degree part of #6719 because we're concerned about the effect of the `declare module` on the Gateway transition.
glasser
added a commit
that referenced
this issue
Aug 8, 2022
Until now, the refactored AS4 did not support Apollo Gateway (or any implementation of the AS3 `gateway` option). That's because `GraphQLRequestContext` is part of the API between Apollo Gateway and Apollo Server, and that type has changed in some minor but incompatible ways in AS4. (Additionally, clashes between `declare module` declarations in AS3 and AS4 caused issue, but we removed those declarations from this branch in PRs #6764 and #6759.) This commit restores gateway support. It does this by having AS4 produce an AS3-style request context object. It uses a new `@apollo/server-gateway-interface` package to define the appropriate types for connecting to Gateway. (Note: this package will be code reviewed in this PR in this repo, but once it's approved, it will move to the apollo-utils repo and be released as non-alpha there. Once we've released AS4.0.0 we can move that package back here, but trying to do some prereleases and some non-prereleases on the same branch seems challenging.) This PR removes the top-level `executor` function, which is redundant with the `gateway` option. (Internally, the relevant field is now named `gatewayExecutor`.) Some types had been parametrized by `TContext`, because in AS3, `GraphQLExecutor` (now `GatewayExecutor`) appeared to take a `<TContext>`. However, even though the type itself took a generic argument, its main use in the return from `gateway.load` implicitly hardcoded the default `TContext`. So we are doubling down on that and only allowing `GraphQLExecutor` to use AS3's default `TContext`, the quite flexible `Record<string, any>`. Most of the way toward #6719.
glasser
added a commit
to apollographql/federation
that referenced
this issue
Aug 8, 2022
Backport of #2044. This removes the last dependency on Apollo Server from Apollo Gateway. Part of apollographql/apollo-server#6057 and apollographql/apollo-server#6719 The new package `@apollo/server-gateway-interface` (in the apollo-utils repo for now, though it should move to the apollo-server repo once AS4 is fully released) defines types that are pretty close to compatible with the AS3 types previously used here, but don't require a dependency on the entirety of AS3. This new package will be used by AS4 and AS4 will convert its data into the format described by these types. This change is entirely a build-time change (other than a slight change to how a enum is referenced). So the worst case scenario if this differs unintentionally from the original AS3 definitions is that users can apply a bit of `as any` to fix it. Note that we've removed some `<TContext>` from types that it turned out were only ever instantiated with `Record<string, any>` anyway. They are left in in RemoteGraphQLDataSource because users making their own data sources can explicitly specify their context type. The types in `@apollo/server-gateway-interface` are pretty close to the AS3 types (with different names) but there are some slight differences. The cache scope enum is replaced with `any`, as enums are not structurally typed and it is otherwise difficult to type them. `GatewayInterface` now expects `onSchemaLoadOrUpdate` to exist and doesn't mention the old `onSchemaChange`. For now, we leave `apollo-reporting-protobuf` alone, so we don't have a direct dependency on a prerelease.
glasser
added a commit
to apollographql/federation
that referenced
this issue
Aug 8, 2022
This removes the last dependency on Apollo Server from Apollo Gateway. Part of apollographql/apollo-server#6057 and apollographql/apollo-server#6719 The new package `@apollo/server-gateway-interface` (in the apollo-utils repo for now, though it should move to the apollo-server repo once AS4 is fully released) defines types that are pretty close to compatible with the AS3 types previously used here, but don't require a dependency on the entirety of AS3. This new package will be used by AS4 and AS4 will convert its data into the format described by these types. This change is entirely a build-time change (other than a slight change to how a enum is referenced). So the worst case scenario if this differs unintentionally from the original AS3 definitions is that users can apply a bit of `as any` to fix it. Note that we've removed some `<TContext>` from types that it turned out were only ever instantiated with `Record<string, any>` anyway. They are left in in RemoteGraphQLDataSource because users making their own data sources can explicitly specify their context type. The types in `@apollo/server-gateway-interface` are pretty close to the AS3 types (with different names) but there are some slight differences. The cache scope enum is replaced with `any`, as enums are not structurally typed and it is otherwise difficult to type them. `GatewayInterface` now expects `onSchemaLoadOrUpdate` to exist and doesn't mention the old `onSchemaChange`. For now, we leave `apollo-reporting-protobuf` alone, so we don't have a direct dependency on a prerelease.
glasser
added a commit
that referenced
this issue
Aug 8, 2022
Until now, the refactored AS4 did not support Apollo Gateway (or any implementation of the AS3 `gateway` option). That's because `GraphQLRequestContext` is part of the API between Apollo Gateway and Apollo Server, and that type has changed in some minor but incompatible ways in AS4. (Additionally, clashes between `declare module` declarations in AS3 and AS4 caused issue, but we removed those declarations from this branch in PRs #6764 and #6759.) This commit restores gateway support. It does this by having AS4 produce an AS3-style request context object. It uses a new `@apollo/server-gateway-interface` package to define the appropriate types for connecting to Gateway. (Note: this package will be code reviewed in this PR in this repo, but once it's approved, it will move to the apollo-utils repo and be released as non-alpha there. Once we've released AS4.0.0 we can move that package back here, but trying to do some prereleases and some non-prereleases on the same branch seems challenging.) This PR removes the top-level `executor` function, which is redundant with the `gateway` option. (Internally, the relevant field is now named `gatewayExecutor`.) Some types had been parametrized by `TContext`, because in AS3, `GraphQLExecutor` (now `GatewayExecutor`) appeared to take a `<TContext>`. However, even though the type itself took a generic argument, its main use in the return from `gateway.load` implicitly hardcoded the default `TContext`. So we are doubling down on that and only allowing `GraphQLExecutor` to use AS3's default `TContext`, the quite flexible `Record<string, any>`. Most of the way toward #6719. Co-authored-by: Trevor Scheer <[email protected]>
glasser
added a commit
to apollographql/federation
that referenced
this issue
Aug 9, 2022
Backport of #2044. Some tweaks for graphql@15 and Node 12 support. This removes the last dependency on Apollo Server from Apollo Gateway. Part of apollographql/apollo-server#6057 and apollographql/apollo-server#6719 The new package `@apollo/server-gateway-interface` (in the apollo-utils repo for now, though it should move to the apollo-server repo once AS4 is fully released) defines types that are pretty close to compatible with the AS3 types previously used here, but don't require a dependency on the entirety of AS3. This new package will be used by AS4 and AS4 will convert its data into the format described by these types. This change is entirely a build-time change (other than a slight change to how a enum is referenced). So the worst case scenario if this differs unintentionally from the original AS3 definitions is that users can apply a bit of `as any` to fix it. Note that we've removed some `<TContext>` from types that it turned out were only ever instantiated with `Record<string, any>` anyway. They are left in in RemoteGraphQLDataSource because users making their own data sources can explicitly specify their context type. The types in `@apollo/server-gateway-interface` are pretty close to the AS3 types (with different names) but there are some slight differences. The cache scope enum is replaced with `any`, as enums are not structurally typed and it is otherwise difficult to type them. `GatewayInterface` now expects `onSchemaLoadOrUpdate` to exist and doesn't mention the old `onSchemaChange`. For now, we leave `apollo-reporting-protobuf` alone, so we don't have a direct dependency on a prerelease.
glasser
added a commit
to apollographql/federation
that referenced
this issue
Aug 9, 2022
This removes the last dependency on Apollo Server from Apollo Gateway. Part of apollographql/apollo-server#6057 and apollographql/apollo-server#6719 The new package `@apollo/server-gateway-interface` (in the apollo-utils repo for now, though it should move to the apollo-server repo once AS4 is fully released) defines types that are pretty close to compatible with the AS3 types previously used here, but don't require a dependency on the entirety of AS3. This new package will be used by AS4 and AS4 will convert its data into the format described by these types. This change is entirely a build-time change (other than a slight change to how a enum is referenced). So the worst case scenario if this differs unintentionally from the original AS3 definitions is that users can apply a bit of `as any` to fix it. Note that we've removed some `<TContext>` from types that it turned out were only ever instantiated with `Record<string, any>` anyway. They are left in in RemoteGraphQLDataSource because users making their own data sources can explicitly specify their context type. The types in `@apollo/server-gateway-interface` are pretty close to the AS3 types (with different names) but there are some slight differences. The cache scope enum is replaced with `any`, as enums are not structurally typed and it is otherwise difficult to type them. `GatewayInterface` now expects `onSchemaLoadOrUpdate` to exist and doesn't mention the old `onSchemaChange`. For now, we leave `apollo-reporting-protobuf` alone, so we don't have a direct dependency on a prerelease.
glasser
added a commit
to apollographql/federation
that referenced
this issue
Aug 9, 2022
This removes the last dependency on Apollo Server from Apollo Gateway. Part of apollographql/apollo-server#6057 and apollographql/apollo-server#6719 The new package `@apollo/server-gateway-interface` (in the apollo-utils repo for now, though it should move to the apollo-server repo once AS4 is fully released) defines types that are pretty close to compatible with the AS3 types previously used here, but don't require a dependency on the entirety of AS3. This new package will be used by AS4 and AS4 will convert its data into the format described by these types. This change is entirely a build-time change (other than a slight change to how a enum is referenced). So the worst case scenario if this differs unintentionally from the original AS3 definitions is that users can apply a bit of `as any` to fix it. Note that we've removed some `<TContext>` from types that it turned out were only ever instantiated with `Record<string, any>` anyway. They are left in in RemoteGraphQLDataSource because users making their own data sources can explicitly specify their context type. The types in `@apollo/server-gateway-interface` are pretty close to the AS3 types (with different names) but there are some slight differences. The cache scope enum is replaced with `any`, as enums are not structurally typed and it is otherwise difficult to type them. `GatewayInterface` now expects `onSchemaLoadOrUpdate` to exist and doesn't mention the old `onSchemaChange`. For now, we leave `apollo-reporting-protobuf` alone, so we don't have a direct dependency on a prerelease.
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Right now, you can't combine AS4 with any released version of Apollo Gateway. This is for a combination of runtime and compile time reasons.
The runtime reason is that the
executor
function returned by AG takes an AS3GraphQLRequestContext
, not an AS4 one.The compile time reason is that Gateway has dependencies on AS3 code and the two
declare module
declarations in AS4 conflict with the AS3 versions.I suspect the best fix for the compile time issue is to just break the dependency via #6057.
My current theory about the best fix for the runtime reason is just to have some code that converts an AS4
GraphQLRequestContext
into an AS3-style one, and leave it there indefinitely (since Gateway is somewhat terminal to be replaced by Router anyway).The text was updated successfully, but these errors were encountered: