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.

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

Open
wants to merge 1 commit into
base: v4
Choose a base branch
from

Conversation

Nargonath
Copy link

@Nargonath Nargonath commented Mar 25, 2024

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

  • My code matches the project's code style and yarn lint:fix passes.
  • I've added tests for the new feature, and yarn test passes.
  • I have detailed the new feature in the relevant documentation.
  • I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
  • If this is a breaking change I've explained why.

Copy link
Member

@benjie benjie left a 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.

Comment on lines +384 to +387
const { parsedResolveInfoFragment, simplifiedFragment } = await graphql(
Schema,
query
);
Copy link
Member

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?

Suggested change
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);
Copy link
Member

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
  }
}

@Nargonath
Copy link
Author

Thanks for the suggestion @benjie. I wasn't aware of the keepRoot option. Does it already exist or is it something you suggest to create as part of this PR?

@benjie
Copy link
Member

benjie commented Apr 1, 2024

It already exists, IIRC it’s what controls the type that is returned. The changes I have suggested are types only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a type guard to distinguish between ResolveTree and FieldsByTypeName
2 participants