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: Add support for better fragment type resolution #199

Merged

Conversation

budde377
Copy link
Collaborator

@budde377 budde377 commented May 30, 2021

This PR adds improved support for fragment resolution by introducing a new map to lookup type relationship

Take the document:

fragment FAuthor on Author {
  __typename
  id
}
fragment FAudience on Audience {
  __typename
  id
  numHands
  isClapping
}
fragment FUser on User {
  id
  __typename
  name
}
query {
  users {
    ...FAuthor
    ...FAudience
    ...FUser
  }
}

and the schema

type Query {
  users: [User]
}

interface User { 
  id: ID!
  name: String
}

type Author implements User {
  id: ID!
  name: String
  numBooks: Int
}

type Audience implements User {
  id: ID!
  name: String
  numHands: Int
  isClapping: Boolean
}

the current implementation will fail to realize which fragment spreads apply to a given type. E.g. that FUser and FAuthor should apply to the Author type. This leads to partial results on fragments and wrong results on inline fragments. By adding some introspection in the shape of the possibleTypeOf lookup map, which maps object types to possible interfaces and unions, we can resolve this.

If the map is omitted, or if the __typename is omitted, this solution will not expand any fragments. This is sound. The current approach expands all fragment spreads and only expands on exact matches for inline fragments, both of which are wrong.

@budde377 budde377 force-pushed the feat-improved-fragment-resolution branch 3 times, most recently from 2ff2fb5 to b759691 Compare May 30, 2021 16:25
@budde377 budde377 force-pushed the feat-improved-fragment-resolution branch 4 times, most recently from 0ac013c to d147401 Compare May 30, 2021 18:39
@budde377 budde377 force-pushed the feat-improved-fragment-resolution branch from d147401 to a3f8930 Compare June 1, 2021 07:26
Copy link
Member

@smkhalsa smkhalsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR!

One question: is there an advantage to using a possibleTypeOf Map instead of a possibleTypes Map? Apollo uses the latter, and it feels more intuitive to define possible types for an interface rather than possible interfaces for a type.

@@ -15,6 +15,7 @@ Map<String, dynamic> operationRootData<TData, TVars>(
request.operation.operationName!,
(request.vars as dynamic).toJson(),
typePolicies,
{},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should include the possible types as an argument to operationRootData and pass them in here.

@budde377
Copy link
Collaborator Author

budde377 commented Jun 1, 2021

One question: is there an advantage to using a possibleTypeOf Map instead of a possibleTypes Map? Apollo uses the latter, and it feels more intuitive to define possible types for an interface rather than possible interfaces for a type.

I don't see any immediate benefits one way or another. Apollo using the latter is a good argument for going in that direction. I'll rewrite!

@budde377 budde377 requested a review from smkhalsa June 1, 2021 18:24
Copy link
Member

@smkhalsa smkhalsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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.

2 participants