Skip to content
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

[version-0.x] Fix TS issues with gateway 0.29+ in AS2 #1964

Closed

Conversation

smyrick
Copy link
Member

@smyrick smyrick commented Jul 6, 2022

If you use gateway version 0.29.0+ with Apollo Server 2 you will start relying on apollo-server-types both ^0.9.0 OR ^3.0.0. See code

apollo-server-types@0.9.0 -> AS2
apollo-server-types@3.0.0 -> AS3

"apollo-server-types": "^0.9.0 || ^3.0.0",

Normally in dependencies this would be ok, that we can use either version, however the underlying dependency apollo-server-env also has a major version change. We updated the GraphQLRequest.headers to instead defined in apollo-server-env to be the Headers from node-fetch. This was an unintentional "breaking change" that got populated. It appears to have no affect on runtime of most users but what it does break is Typescript compile steps. So if users have a tsc step they can't even use these versions together.

My fix is to instead have npm pull in the dependency of one or the other, we pull in both libs and define in the typed source code that the types themselves could be one or the other.

I believe we should also apply this fix to main if we want @apollo/gateway v2 to still work with AS2.

@netlify
Copy link

netlify bot commented Jul 6, 2022

👷 Deploy request for apollo-federation-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit e1540c9

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 6, 2022

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.

@smyrick
Copy link
Member Author

smyrick commented Jul 6, 2022

I could use some other eyes on this because I am not sure if this is still considered a major change since I did update the typing info in a lot of places

Copy link
Contributor

@benweatherman benweatherman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐐

I believe we should also apply this fix to main if we want @apollo/gateway v2 to still work with AS2.

AS2 doesn't support graphql@16, and @apollo/gateway@2 requires it, so I think we're OK to keep this v0.x only.

As for breaking change or not, since these are mostly types my hunch is that they get compiled away. Any code that's working today should continue to work. We can check the built output to make sure the differences aren't breaking.

.gitignore Outdated Show resolved Hide resolved
gateway-js/src/config.ts Outdated Show resolved Hide resolved
@@ -116,11 +127,15 @@ export class RemoteGraphQLDataSource<
// Special handling of cache-control headers in response. Requires
// Apollo Server 3, so we check to make sure the method we want is
// there.

// @ts-ignore
const cachePolicy = options.incomingRequestContext?.overallCachePolicy;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to ignore this because TS is complaining the field doesn't exist in types from AS2, which was hidden before.

@benweatherman
Copy link
Contributor

Here's the diff of this code vs @apollo/gateway@0.51.0. It seems like mostly types, other than a few JS changes (which were changed in the typescript code, so they make sense).

Mind double-checking to make sure I didn't miss anything?

diff -bur --exclude '*.map' dist/config.d.ts dist-shane/config.d.ts
--- dist/config.d.ts	1985-10-26 03:15:00.000000000 -0500
+++ dist-shane/config.d.ts	2022-07-07 19:01:24.000000000 -0500
@@ -1,9 +1,8 @@
 import { GraphQLError, GraphQLSchema } from 'graphql';
 import { HeadersInit } from 'node-fetch';
-import { GraphQLRequestContextExecutionDidStart } from 'apollo-server-types';
 import type { Logger } from '@apollo/utils.logger';
 import { ServiceDefinition } from '@apollo/federation';
-import { GraphQLDataSource } from './datasources/types';
+import { GraphQLDataSource, GraphQLRequestContextExecutionDidStart } from './datasources/types';
 import { QueryPlan } from '@apollo/query-planner';
 import { OperationContext } from './operationContext';
 import { ServiceMap } from './executeQueryPlan';
diff -bur --exclude '*.map' dist/datasources/RemoteGraphQLDataSource.d.ts dist-shane/datasources/RemoteGraphQLDataSource.d.ts
--- dist/datasources/RemoteGraphQLDataSource.d.ts	1985-10-26 03:15:00.000000000 -0500
+++ dist-shane/datasources/RemoteGraphQLDataSource.d.ts	2022-07-07 19:01:24.000000000 -0500
@@ -1,8 +1,13 @@
-import { GraphQLRequestContext, GraphQLResponse, ValueOrPromise } from 'apollo-server-types';
+import { ValueOrPromise } from 'apollo-server-types';
 import { ApolloError } from 'apollo-server-errors';
-import { GraphQLDataSource, GraphQLDataSourceProcessOptions } from './types';
+import { GraphQLDataSource, GraphQLDataSourceProcessOptions, GraphQLRequest, GraphQLResponse } from './types';
 import { Request as NodeFetchRequest } from 'node-fetch';
 import { Fetcher, FetcherResponse } from '@apollo/utils.fetcher';
+export declare type RequiredGraphQLRequestContext<TContext> = {
+    request: GraphQLRequest;
+    response: GraphQLResponse;
+    context: TContext;
+};
 export declare class RemoteGraphQLDataSource<TContext extends Record<string, any> = Record<string, any>> implements GraphQLDataSource<TContext> {
     fetcher: Fetcher;
     constructor(config?: Partial<RemoteGraphQLDataSource<TContext>> & object & ThisType<RemoteGraphQLDataSource<TContext>>);
@@ -13,7 +18,7 @@
     private sendRequest;
     willSendRequest?(options: GraphQLDataSourceProcessOptions<TContext>): ValueOrPromise<void>;
     private respond;
-    didReceiveResponse?(requestContext: Required<Pick<GraphQLRequestContext<TContext>, 'request' | 'response' | 'context'>>): ValueOrPromise<GraphQLResponse>;
+    didReceiveResponse?(requestContext: RequiredGraphQLRequestContext<TContext>): ValueOrPromise<GraphQLResponse>;
     didEncounterError(error: Error, _fetchRequest: NodeFetchRequest, _fetchResponse?: FetcherResponse, _context?: TContext): void;
     parseBody(fetchResponse: FetcherResponse, _fetchRequest?: NodeFetchRequest, _context?: TContext): Promise<object | string>;
     errorFromResponse(response: FetcherResponse): Promise<ApolloError>;
diff -bur --exclude '*.map' dist/datasources/RemoteGraphQLDataSource.js dist-shane/datasources/RemoteGraphQLDataSource.js
--- dist/datasources/RemoteGraphQLDataSource.js	1985-10-26 03:15:00.000000000 -0500
+++ dist-shane/datasources/RemoteGraphQLDataSource.js	2022-07-07 19:01:24.000000000 -0500
@@ -4,7 +4,7 @@
 };
 Object.defineProperty(exports, "__esModule", { value: true });
 exports.RemoteGraphQLDataSource = void 0;
-const apollo_server_types_1 = require("apollo-server-types");
+const apollo_server_types_3_1 = require("apollo-server-types-3");
 const apollo_server_errors_1 = require("apollo-server-errors");
 const predicates_1 = require("../utilities/predicates");
 const types_1 = require("./types");
@@ -47,10 +47,11 @@
             throw new Error('Missing query');
         }
         const { query, ...requestWithoutQuery } = request;
+        const cachePolicy = (_b = options.incomingRequestContext) === null || _b === void 0 ? void 0 : _b.overallCachePolicy;
         const overallCachePolicy = this.honorSubgraphCacheControlHeader &&
             options.kind === types_1.GraphQLDataSourceRequestKind.INCOMING_OPERATION &&
-            ((_b = options.incomingRequestContext.overallCachePolicy) === null || _b === void 0 ? void 0 : _b.restrict)
-            ? options.incomingRequestContext.overallCachePolicy
+            (cachePolicy === null || cachePolicy === void 0 ? void 0 : cachePolicy.restrict)
+            ? cachePolicy
             : null;
         if (this.apq) {
             const apqHash = (0, utils_createhash_1.createHash)('sha256').update(request.query).digest('hex');
@@ -129,10 +130,10 @@
                 hint.maxAge = +maxAge;
             }
             if (parsed['private'] === true) {
-                hint.scope = apollo_server_types_1.CacheScope.Private;
+                hint.scope = apollo_server_types_3_1.CacheScope.Private;
             }
             if (parsed['public'] === true) {
-                hint.scope = apollo_server_types_1.CacheScope.Public;
+                hint.scope = apollo_server_types_3_1.CacheScope.Public;
             }
             overallCachePolicy.restrict(hint);
         }
diff -bur --exclude '*.map' dist/datasources/types.d.ts dist-shane/datasources/types.d.ts
--- dist/datasources/types.d.ts	1985-10-26 03:15:00.000000000 -0500
+++ dist-shane/datasources/types.d.ts	2022-07-07 19:01:24.000000000 -0500
@@ -1,4 +1,9 @@
-import { GraphQLResponse, GraphQLRequestContext } from 'apollo-server-types';
+import { GraphQLRequest as GraphQLRequest1, GraphQLResponse as GraphQLResponse1, GraphQLRequestContext as GraphQLRequestContext1, GraphQLRequestContextExecutionDidStart as GraphQLRequestContextExecutionDidStart1 } from 'apollo-server-types';
+import { GraphQLRequest as GraphQLRequest3, GraphQLResponse as GraphQLResponse3, GraphQLRequestContext as GraphQLRequestContext3, GraphQLRequestContextExecutionDidStart as GraphQLRequestContextExecutionDidStart3 } from 'apollo-server-types-3';
+export declare type GraphQLRequest = GraphQLRequest1 | GraphQLRequest3;
+export declare type GraphQLResponse = GraphQLResponse1 | GraphQLResponse3;
+export declare type GraphQLRequestContext<TContext> = GraphQLRequestContext1<TContext> | GraphQLRequestContext3<TContext>;
+export declare type GraphQLRequestContextExecutionDidStart<TContext> = GraphQLRequestContextExecutionDidStart1<TContext> | GraphQLRequestContextExecutionDidStart3<TContext>;
 export interface GraphQLDataSource<TContext extends Record<string, any> = Record<string, any>> {
     process(options: GraphQLDataSourceProcessOptions<TContext>): Promise<GraphQLResponse>;
 }
@@ -8,7 +13,7 @@
     LOADING_SCHEMA = "loading schema"
 }
 export declare type GraphQLDataSourceProcessOptions<TContext extends Record<string, any> = Record<string, any>> = {
-    request: GraphQLRequestContext<TContext>['request'];
+    request: GraphQLRequest;
 } & ({
     kind: GraphQLDataSourceRequestKind.INCOMING_OPERATION;
     incomingRequestContext: GraphQLRequestContext<TContext>;
diff -bur --exclude '*.map' dist/executeQueryPlan.d.ts dist-shane/executeQueryPlan.d.ts
--- dist/executeQueryPlan.d.ts	1985-10-26 03:15:00.000000000 -0500
+++ dist-shane/executeQueryPlan.d.ts	2022-07-07 19:01:24.000000000 -0500
@@ -1,6 +1,6 @@
-import { GraphQLExecutionResult, GraphQLRequestContext, VariableValues } from 'apollo-server-types';
+import { GraphQLExecutionResult, VariableValues } from 'apollo-server-types';
 import { GraphQLFieldResolver } from 'graphql';
-import { GraphQLDataSource } from './datasources/types';
+import { GraphQLDataSource, GraphQLRequestContext } from './datasources/types';
 import { OperationContext } from './operationContext';
 import { FetchNode, QueryPlan } from '@apollo/query-planner';
 export declare type ServiceMap = {
diff -bur --exclude '*.map' dist/index.d.ts dist-shane/index.d.ts
--- dist/index.d.ts	1985-10-26 03:15:00.000000000 -0500
+++ dist-shane/index.d.ts	2022-07-07 19:01:24.000000000 -0500
@@ -1,9 +1,9 @@
 import { GraphQLService, Unsubscriber } from 'apollo-server-core';
-import { GraphQLExecutionResult, GraphQLRequestContextExecutionDidStart } from 'apollo-server-types';
+import { GraphQLExecutionResult } from 'apollo-server-types';
 import { GraphQLSchema } from 'graphql';
 import { buildOperationContext } from './operationContext';
 import { executeQueryPlan, ServiceMap } from './executeQueryPlan';
-import { GraphQLDataSource } from './datasources/types';
+import { GraphQLDataSource, GraphQLRequestContextExecutionDidStart } from './datasources/types';
 import { ServiceEndpointDefinition, Experimental_DidFailCompositionCallback, Experimental_DidResolveQueryPlanCallback, Experimental_DidUpdateSupergraphCallback, Experimental_UpdateComposition, CompositionInfo, GatewayConfig } from './config';
 import { IntrospectAndCompose, LocalCompose } from './supergraphManagers';
 declare type DataSourceMap = {
@@ -81,7 +81,7 @@
     private updateWithSchemaAndNotify;
     serviceHealthCheck(serviceMap?: DataSourceMap): Promise<{
         name: string;
-        response: import("apollo-server-types").GraphQLResponse;
+        response: import("./datasources/types").GraphQLResponse;
     }[]>;
     private serviceListFromSupergraphSdl;
     private serviceListFromComposedSchema;

@smyrick
Copy link
Member Author

smyrick commented Jul 12, 2022

You can reproduce the error here running npm run compile

https://stackblitz.com/edit/apolloserver-2-with-gateway-45?file=README.md

I am pulling this setup and this PR package to test locally

readonly requestContext: GraphQLRequestContextExecutionDidStart<
Record<string, any>
>;
readonly requestContext: GraphQLRequestContext<Record<string, any>>;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a change in type because we don't need the guarantee of the other fields here, only the request. Plus this now matches up with the other usages

@smyrick
Copy link
Member Author

smyrick commented Jul 19, 2022

I had to fix the types in make-fetch-happen first: DefinitelyTyped/DefinitelyTyped#61233

@smyrick
Copy link
Member Author

smyrick commented Aug 15, 2022

This PR has drastically been affected by this PR: #2050

While using gateway 0.52.0 does not solve the TS issues with Apollo Server 2 the changes have become so big that I am going to completely open a new branch. The changes will be here for reference though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants