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.

Sign up for GitHub

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

Jump to bottom

[BREAKING] Expand didEncounterErrors API #4380

Closed
wants to merge 1 commit into from
Closed

[BREAKING] Expand didEncounterErrors API #4380

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions packages/apollo-gateway/src/datasources/RemoteGraphQLDataSource.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,11 @@ export class RemoteGraphQLDataSource<TContext extends Record<string, any> = Reco
body: JSON.stringify(requestWithoutHttp),
});

let httpResponse: Response | undefined;

try {
// Use our local `fetcher` to allow for fetch injection
const httpResponse = await this.fetcher(httpRequest);
httpResponse = await this.fetcher(httpRequest);

if (!httpResponse.ok) {
throw await this.errorFromResponse(httpResponse);
Expand All @@ -164,7 +166,7 @@ export class RemoteGraphQLDataSource<TContext extends Record<string, any> = Reco
http: httpResponse,
};
} catch (error) {
this.didEncounterError(error, httpRequest);
this.didEncounterError?.({ error, request: httpRequest, response: httpResponse, context });
throw error;
}
}
Expand All @@ -183,9 +185,9 @@ export class RemoteGraphQLDataSource<TContext extends Record<string, any> = Reco
>,
): ValueOrPromise<GraphQLResponse>;

public didEncounterError(error: Error, _request: Request) {
throw error;
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand what the original intention of this throw was, but I like the idea of removing it in the default implementation!

}
public didEncounterError?(
requestContextWithError: { error: Error, response?: Response, context: TContext, request: Request }
Copy link
Member

@abernix abernix Jul 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, a lengthy comment I wrote got on this line was lost (possibly because of a bug between the keyboard and the chair) and I cannot recover it. I'll try to come back to this later, but please forgive my lack of time to re-read my re-cobbled together from clipboard history response right now:

The object that we colloquially refer to as the "request context" is the GraphQLRequestContext. It takes on a number of more narrowed forms further down in the apollo-server-types file. The user context, which is TContext in this signature proposal, is on the context property of that GraphQLRequestContext<TContext>.

The object you are proposing that we pass here to didEncounterError as requestContextWithError is not that "request context" (or even really a variation of it with merely error attached as the name suggests). Specifically, the request and response are (HTTP!) apollo-server-env.Request and apollo-server-env.fetch.Response, respectively (which are what I think you intended on exposing!), whereas a GraphQLRequestContext would contain { request: GraphQLRequest, response: GraphQLResponse }) (i.e., GraphQLRequest, GraphQLResponse).

I agree with the idea of unifying the API (and this didEncounterError has always surprised me), but I don't think accomplishes unification now. For now, another approach is that we merely add response: http.Response as the third parameter and not introduce a breaking change, barring further API design discussion (that I think is warranted, but in the name of time, third param seems not too controversial, particularly since the response is optional).

If it is critical to also have user context (or it's important to have other GraphQLRequestContext-y things, like logger!) today and it warrants a breaking change (which comes with the burden of making a more clear CHANGELOG with this PR that explains the change to consumers but also, the burden of asking existing implementors to make the change), perhaps another approach could be to have the well-known requestContext as the first argument and the HTTP-transport specific properties as their own secondary parameter object, e.g.:

    requestContext: GraphQLRequestContext<TContext>,
    http: { error: Error, response: Response, request: Request },`

Though will need to pause and consider whether we want the pattern to be "The request context is always the last argument", which can sometimes be more durable for other-use-cases, or if we'd prefer that it be the first parameter. I think there is existing art to consider in the plugin API on willResolveField that intentionally breaks the pattern of first-arg GraphQLRequestContext because of the natural tiering of the nested life-cycle hooks which the Gateway does not have.

Anyhow, long-story-longer, but with less precision because I deleted everything and tried to ramble my way back through it: Maybe we can just add response as the third argument? (And probably rename both the positional parameters to httpRequest and httpResponse to reduce confusion?

): ValueOrPromise<void>;

public parseBody(
response: Response,
Expand Down