[BREAKING] Expand didEncounterErrors API

[michael-watson]

• Include http Response in didEncounterErrors if available
• Unified API to match other RemoteGraphQLDataSource hooks

[jhampton]

FWIW, I’d like to propose using an interface and destructing a single argument for the Gateway hooks as a part of the experimental -> production API issue. I mention that here because moving to single-argument methods has two benefits:

• Fewer breaking changes when adding new properties (like this one)
• Easier use of FP conventions when composing, reducing, currying functions

Thoughts?

[abernix]

From a conceptual standpoint, it would be great it this PR came coupled with a demonstrated use-case of what needs to be accomplished (e.g. Observability, etc.) in the form of an issue so we could discuss the options.
For example, if it's observability, it'd be nice to think about how related functionality (like errorFromResponse) might tie into that use-case, whether it'd been considered, etc.

If time is not of the essence, I'm happy to discuss this further over time but assuming that response is the desired integration point (I'm assuming, since it's not stated here), I'd mostly rather avoid the breaking change altogether right now and just add response: fetch.Response on the end if that does the trick (as I've noted below in my line-feedback). If it also needs to include context then perhaps we should actually make requestContext: GraphQLRequestContext the first argument and use a separate argument object entirely for the HTTP-specific properties.

If we take that route, we should leave an issue open labeled plugins and federation to address this in a more exhaustive project.

[abernix]

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!

[abernix]

Ok, looks like I lost a somewhat sizable comment on this PR. I will try to recover it!

[abernix]

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?

[abernix]

In case it unblocks anyone immediately while we sort out the details, I published @apollo/[email protected] with the addition of response as the third parameter (i.e., non-breaking) in 8ff5609.

[abernix]

Hi @michael-watson!,

This is a semi-automated message. Apologies for the lack of personal detail. I’d encourage reading the rest of this message but, if you're busy, hopefully this is as easy for you — or someone who is willing to own the contribution — as pushing the button later in this message to re-create this PR, from an auto-migrated branch, on the new Federation repository. If you have time, read on!

We’re in the process of moving federation-related concerns out of this repository (where it has historically lived) into a new home specifically for Apollo Federation. This PR is affected by the transition since it touches code which has been moved.

I’ve done some preparations to make this as easy as possible, but we’ll need to close this PR, and I could use your help in re-opening it on the new Apollo Federation repository.

In apollographql/federation#134 (which has a lot more details, if you’re interested), we introduced the same code that was in this repository to the new Federation repository (maintaining its history) and removed the code from this repository in #4561.

While this PR still needs to get reviewed and approved to land, it should no longer live in this repository in its current state since it cannot merge in anymore. To facilitate the movement of this PR to the new home, I’ve automatically generated branches on the new repository using the same commit authorship and messages that you originally included on this PR.

Pull-requests can’t be moved on GitHub in the same way Issues can be. While I could just create the PRs from these new branches too, the contribution belongs to you!

---

Original PR author only: Click this button to open a new PR from the auto-created apollo-server-import/pr-4380 branch on the new Apollo Federation repository

(The code and your commits should be the same and you will have an opportunity to confirm, but this way you can continue to be the author and track its progress on the new Federation repository!)

---

There’s no easy way to bring over any existing comments on this PR, so I encourage you to copy and paste those onto the new issue if possible.

Overall, I hope this process is relatively easy for you while maintaining your commit contribution authorship. I apologize for any inconvenience caused by this shuffle, but please ping me if I can help in any way!

🚀