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

Feature: add support for interface resolution #21

Merged

Conversation

MatteoGranziera
Copy link
Contributor

This implementation will enable the resolution of a GraphQL interface requested by the federation.

The resolution will be accessible using the entity resolution by putting as typename the interface as the following example

Node schema

type Query {
  product: Product
}

interface Product @key(fields: "id") {
  id: ID!
  name: String!
}

type BundleProduct implements Product @key(fields: "id") {
  id: ID!
  name: String!
  productIds: [ID!]!
}

Query

query {
    _entities(representations: [{ __typename: "Product", id: "1"}]) {
      __typename
      ... on Product {
        id
        name
      }
      ... on BundleProduct {
        productIds
      }
    }
  }

Note
The current implementation of mercurius does not allow to define an interface without its implementations but it could be a future improvement. Anyway it's possible to use this feature by import the @interfaceObject directive from Apollo Federation v2.3

Copy link
Contributor

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm


if (type && 'then' in type && typeof type.then === 'function') {
return type.then((resolvedType) => {
const resolveReference = resolvedType.resolveReference
Copy link
Contributor

@marco-ippolito marco-ippolito Mar 29, 2023

Choose a reason for hiding this comment

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

Suggested change
const resolveReference = resolvedType.resolveReference
const resolveReference = resolvedType.resolveReference || function defaultResolveReference () {
return reference
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

lib/federation.js Outdated Show resolved Hide resolved
lib/federation.js Outdated Show resolved Hide resolved

const type = info.schema.getType(__typename)

if (!type || !(isObjectType(type) || isInterfaceType(type))) {
Copy link
Contributor

@marco-ippolito marco-ippolito Mar 29, 2023

Choose a reason for hiding this comment

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

I dont understand this:
first we check isInterfaceType(type) and we throw
then we check !isInterfaceType(type) and we return

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first check verifies that the type is allowed to continue the type resolution (should be an object or an interface). The second if discriminate between the object and the interface, when is an object type there is no need to call resolveType function because the typename is already defined.

lib/federation.js Outdated Show resolved Hide resolved
}

const resolveTypeFn = type.resolveType
? type.resolveType
Copy link
Contributor

Choose a reason for hiding this comment

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

we can shorthand this

  const resolveTypeFn = type.resolveType
    ? type.resolveType
    : defaultTypeResolver

with

 const resolveTypeFn = type.resolveType || defaultTypeResolver;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

simplify promise checks

Co-authored-by: Marco Ippolito <marcoippolito54@gmail.com>
Copy link
Collaborator

@codeflyer codeflyer left a comment

Choose a reason for hiding this comment

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

LGTM

@MatteoGranziera MatteoGranziera requested review from marco-ippolito and removed request for jonnydgreen March 29, 2023 12:32
Copy link
Contributor

@marco-ippolito marco-ippolito left a comment

Choose a reason for hiding this comment

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

LGTM (@mcollina I dont have permissions on this repo for some reason)

@MatteoGranziera
Copy link
Contributor Author

I've removed by accident jonnygreen as reviewer
@mcollina

Copy link

@jonnydgreen jonnydgreen left a comment

Choose a reason for hiding this comment

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

Apologies for the delay in reviewing, LGTM!

@mcollina mcollina merged commit 626361d into mercurius-js:main Apr 8, 2023
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.

5 participants