add the operation kind to FetchNode #1427
Conversation
this will allow data sources to decide if they can cache or deduplicate a request
|
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. |
There was a problem hiding this comment.
This PR LGTM (I left one review comment), but I'll defer to other reviewers (and I'll add @pcmanus as a requested reviewer).
My one suggestion not related to the code is that I'd update the PR body with a bit of additional "motivation" that says: "...without needing to parse the operation to find out".
Anyhow, that's just a nit. @pcmanus, @martijnwalraven, what do you think?
query-planner-js/src/buildPlan.ts
Outdated
| @@ -801,6 +801,7 @@ class FetchGroup { | |||
| requires: inputNodes ? trimSelectionNodes(inputNodes.selections) : undefined, | |||
| variableUsages: this.selection.usedVariables().map(v => v.name), | |||
| operation: stripIgnoredCharacters(print(operation)), | |||
| operationKind: (operation.definitions[0] as OperationDefinitionNode).operation, | |||
There was a problem hiding this comment.
This is the only thing I find myself scrutinizing at a high-level — though I haven't checked this branch out to look more closely so apologies if this is obvious to others:
Is there a guarantee that the (first) operation definition that this points to is indeed the operation that's being executed? In a non-GraphQL query, I would be worried about this since the operation that we'd actually execute would be determined by the operationName, but since this is probably a generated query and we probably don't generate multi-operation documents, it's probably fine?
There was a problem hiding this comment.
I have not seen batching on subgraph requests, but I might be wrong on that
There was a problem hiding this comment.
it could be replaced with a more explicit "has_mutations", maybe?
There was a problem hiding this comment.
There cannot be multiple operations, but there can be fragments. As it happens, the code currently guarantees the operation will be first in that case so the code would work as is, but it's fragile to rely on it and I'd rather change that line. The shortest solution is probably just:
operationKind: this.isEntityFetch ? 'query' : this.rootKindbut the "cleanest" solution, which is not much more complicated, is probably to change operationForEntitiesFetch and operationForQueryFetch to return the Operation directly, and then to have:
const fetchNode: FetchNode = {
...,
operation: stripIgnoredCharacters(print(operationToDocument(operation))),
operationKind: operation.rootKind
};Happy to write the commit for what if you prefer, though I'm just you shouldn't have much difficulty :).
This is expected since the [Ref]erenced PR introduced it! Ref: apollographql/federation#1427
* router-bridge: Update to `@apollo/query-planner@2.0.0-alpha.6` Most notably to bring in apollographql/federation#1511 Ref: apollographql/federation@ee84b3fd9df161c3a94 * tests: update snapshots to include `operationKind` This is expected since the [Ref]erenced PR introduced it! Ref: apollographql/federation#1427
Fix #1423
this will allow data sources to decide if they can cache or deduplicate
a request