chore: typescript-redux followups
#656
Conversation
|
is this looking for review yet? |
|
@glasser you're welcome to, but can also wait til I've addressed the other line items. I'd lean towards just waiting unless you're looking for something to do. |
…dCharacters from graphql-js
Validate their definition length === 1. Also: * Update sibling fns in all 3 packages and add a note stating their existence in other places. * Remove extraneous 'query' in parsing function
This didn't make sense. Rather than falling back to something that isn't correct, we should just assert the existence of the thing that is expected to always be there.
trevor-scheer
force-pushed
the
trevor/redux-followups
branch
from
8987755
to
fb0f6e9
Compare
| @@ -200,6 +200,10 @@ function mergeFieldNodeSelectionSets( | |||
| (node): node is FieldNode => node.kind === Kind.FIELD, | |||
| ); | |||
|
|
|||
| // Committed by @trevor-scheer but authored by @martijnwalraven | |||
There was a problem hiding this comment.
I think @martijnwalraven might be interested in looking over these comments and making sure they fully capture what he wants, but that could be after this PR merges.
|
Made it through all the commits. Have a number of small comments. Confirmed that all the things you checked off on the checklist are addressed (modulo my comments). |
Also move some generic utils out of the composition utils file and into their own tiny files in a utilities folder. This more closely matches the structure of the gateway's code as well.
trevor-scheer
force-pushed
the
trevor/redux-followups
branch
from
385dc02
to
e55c683
Compare
trevor-scheer
force-pushed
the
trevor/redux-followups
branch
from
e55c683
to
f85a597
Compare
There was a problem hiding this comment.
Looks real good. Thanks for the thorough follow-through and clarity here!
(Agree with opportunity to use apollo-graphql for some utilities – which I recently consolidated some into from apollo-env, but I'm also comfortable with the duplication of these super small specialized utilities since they're unlikely to drift.)
| * @returns A parsed FieldSet | ||
| */ | ||
| export function parseSelections(source: string): ReadonlyArray<SelectionNode> { | ||
| const parsed = parse(`{${source}}`); |
There was a problem hiding this comment.
Could be an opportunity to provide a more useful error message in the case of errors vai this parseSelections function, or possibly some specialized utility functions that produce valuable error messages in the event of parse errors; I'm just going to put this on the same radar by linking to this:
e.g., perhaps a parseKeys, parseRequires, parseProvides might wrap this, or perhaps this can take a param.
Just a suggestion, not asking for it in this PR.
trevor-scheer
modified the milestones:
@apollo/gateway v0.27.0,
@apollo/gateway v0.26.2
Copying (mostly) from #624:
_1[ ] change composition (and spec) to only allow one(Martijn: I don't believe this is what we want)@join__typeper graphundefinedfrom it and improve error messages in the places that this change makes not compileFieldSetprintWithReducedWhitespaceencouraging us to considerstripIgnoredCharacters?