-
-
Notifications
You must be signed in to change notification settings - Fork 129
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
feat(graphql-parse-resolve-info): add isResolveTree type guard #858
base: v4
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A safe way to know if the result is a ResolveTree or FieldsByTypeName is to know whether you called the function with keepRoot
set or not. Maybe it would be better to add a function overload to parseResolveInfo
, e.g.:
export function parseResolveInfo(
resolveInfo: GraphQLResolveInfo,
options?: Omit<ParseOptions, "keepRoot"> & { keepRoot?: false }
): ResolveTree | null | undefined;
export function parseResolveInfo(
resolveInfo: GraphQLResolveInfo,
options: Omit<ParseOptions, "keepRoot"> & { keepRoot: true }
): FieldsByTypeName;
export function parseResolveInfo(
resolveInfo: GraphQLResolveInfo,
options: ParseOptions
): ResolveTree | FieldsByTypeName | null | undefined;
export function parseResolveInfo(
resolveInfo: GraphQLResolveInfo,
options: ParseOptions = {}
): ResolveTree | FieldsByTypeName | null | undefined {
// ...
}
This way you wouldn't even need the type guard, it would be typed for you automatically? If this seems like a feasible approach, please consider updating the PR to take this approach instead.
const { parsedResolveInfoFragment, simplifiedFragment } = await graphql( | ||
Schema, | ||
query | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might have mis-typed this example, we wouldn't get these results from calling graphql()
?
Maybe something like this would be more helpful?
const { parsedResolveInfoFragment, simplifiedFragment } = await graphql( | |
Schema, | |
query | |
); | |
const myResolver: GraphQLFieldResolver<any, any> = (source, args, context, resolveInfo) => { | |
const parsedResolveInfoFragment = parseResolveInfo(resolveInfo); | |
if (!isResolveTree(parsedResolveInfoFragment)) { | |
throw new Error(`Expected ResolveTree`); | |
} | |
const simplifiedFragment = simplifyParsedResolveInfoFragmentWithType( | |
parsedResolveInfoFragment, | |
resolveInfo.returnType | |
); | |
// ... | |
}; |
export function isResolveTree( | ||
value: ResolveTree | FieldsByTypeName | null | undefined | ||
): value is ResolveTree { | ||
return typeof value?.name === "string" && Boolean(value.fieldsByTypeName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is safe; depending on how it's used it could be tricked by a query like:
{
field {
name: id # `ID` type is always a string
fieldsByTypeName: __typename
}
}
Thanks for the suggestion @benjie. I wasn't aware of the |
It already exists, IIRC it’s what controls the type that is returned. The changes I have suggested are types only. |
Fix #849
Description
This PR adds a TypeScript type guard to determine whether a value is a
ResolveTree
or not.Performance impact
None (or minimal), it's just a two property read and boolean conditions.
Security impact
none
Checklist
yarn lint:fix
passes.yarn test
passes.RELEASE_NOTES.md
file (if one exists).