Register TermNode connections on ContentNode interface, not individual types

[chriszarate]

What does this implement/fix? Explain your changes.

Testing the waters a bit on moving certain connections to interfaces instead of individual types. There is a limit on how much of this we can do while still retaining the information encoded by a good schema, but I think it makes sense for generic connections like terms and contentNodes.

Therefore, this PR registers the TermNode connection (terms) on the ContentNode interface, instead of on each individual post type.

There is a slight change in result here: A post type will receive the terms connection even if it does not have any taxonomies registered against it. While this removes a small bit of information from the schema, I think the tradeoff is worth it, because:

1. The more specific connections still exist (e.g., Post->Tag or Post->Category) and continue to convey the information that was removed.
2. The ContentNode->TermNode connections resolves to the TermNode type, which contains all of the available taxonomies, not just the ones registered to that post type. It doesn't really make sense to provide specificity only in the case where no taxonomies are registered. It is a generic connection, so let it be generic.
3. There is useful symmetry in having a ContentNode->TermNode connection and a TermNode->ContentNode connection.

[codeclimate]

Code Climate has analyzed commit 9f6ec6b and detected 0 issues on this pull request.

View more on Code Climate.

[coveralls]

Coverage decreased (-0.01%) to 79.369% when pulling 9f6ec6b on chriszarate:chore/register-terms-on-interface into fb39463 on wp-graphql:develop.

[jasonbahl]

We (@chriszarate and I) talked a lot about this in Slack and I want to add some context of that conversation.

I'm hesitant to add this as I feel like it exposes something in the Schema that shouldn't be exposed.

If I were to register a post type with no taxonomy connections, but the schema showed that I can query:

{ 
  myCustomTypeWithNoTaxonomyConnection { 
    terms { 
      nodes { 
        ... 
      } 
     } 
   } 
 }

That feels misleading as far as "Schema as documentation" is concerned. We know for a fact that MyCustomTypeWithNo TaxonomyConnection has no connection to taxonomies, so teasing this connection that will always return null feels wrong.

It's one thing to have nullable types that are sometimes null and sometimes have value, but in this case we know ahead of time that the type will have no terms.

I think it's irresponsible and misleading to expose this to the Schema.

The Schema should show what's possible, and in this case we know that's not possible.

After our conversation in Slack, I opened issue #2027 to discuss some ideas related to this.

I think the general idea this PR from @chriszarate is trying to solve is having more general queries that can be used across types.

It's annoying to have to write similar, but slightly different, queries for Page, Post, CustomPostType, etc.

If we introduce a concept of NodeWith${ConnectionName} and apply that Interface to all Types that support said connection, it would allow for more flexible, re-usable queries.

Consider even core WordPress Post Types:

• Page: no taxonomy associations (at least out of the box)
• Post: supports category and tag taxonomies

With the NodeWith${ConnectionName} convention, we could apply the following Interfaces to the Post type: NodeWithTermNodeConnection, NodeWithTagConnection, NodeWithCategoryConnection.

And leave the Page Type alone.

And now we could write a resilient query like:

{
  post( id: "id-of-post" ) {
    ...ResilientFragment
  }
  page( id: "id-of-page" ) {
    ...ResilientFragment
  }
}

fragment ResilientFragment on ContentNode {
  id
  uri
  ...on NodeWithTitle {
    title
  }
  ...on NodeWithCategoryConnection {
    categories {
      nodes {
        id
        name
      }
    }
  }
  ...on NodeWithTagConnection {
    tags {
      nodes {
        id
        name
      }
    }
  }
  ...on NodeWithTermsConnection {
    terms {
      nodes {
        id
        name
      }
    }
  }
}

We could even go a step further, and apply the TermNodeConnection to the TagConnection and CategoryConnection connections, and simplify the query even further:

  post( id: "id-of-post" ) {
    ...ResilientFragment
  }
  page( id: "id-of-page" ) {
    ...ResilientFragment
  }
}

fragment ResilientFragment on ContentNode {
  id
  uri
  ...on NodeWithTitle {
    title
  }
  ...on NodeWithCategoryConnection {
    categories {
      ...TermNodes
    }
  }
  ...on NodeWithTagConnection {
    tags {
      ...TermNodes
    }
  }
  ...on NodeWithTermsConnection {
    terms {
      ...TermNodes
    }
  }
}

fragment TermNodes on TermsConnection {
  nodes {
    id
    name
  }
}

I believe this helps with the intent of this PR (creating re-usable, resilient fragments) but also keeps the integrity of the Schema as documentation aspect.

[chriszarate]

If we introduce a concept of NodeWith${ConnectionName} and apply that Interface to all Types that support said connection, it would allow for more flexible, re-usable queries.

I'm on board with this approach outlined here and in #2027, even if it complicates the generic queries that I find valuable. Should I retool this PR to introduce NodeWithTermConnection?

I'm a tiny bit concerned about what this looks like when consuming a query like this from a strongly typed language, but I can work on understanding that better in parallel.

[jasonbahl]

@chriszarate my thought is that this might actually be the responsibility of the register_graphql_connection() API.

At least to some level. 🤔

And another thought is that we could have some sort of connection that returns an Interface comprised of Taxonomies actually associated with the taxonomy.

Like we have a ContentTypesOf${TaxonomyName} Enum currently:,

And I think we could probably implement something similar for Connection Interfaces. 🤔 🤔

[jasonbahl]

@chriszarate my thought is that this might actually be the responsibility of the register_graphql_connection() API.

What I mean by this is that the FromType of every connection (not just Post/Terms) should get an Interface like this.

So if I register a connection from User to Post like so:

register_graphql_connection( [
  'fromType' => 'User',
  'toType' => 'Post',
  'fromFieldName' => 'featuredPosts',
  'description' => __( 'Featured posts by this author', 'text-domain' ),
   ...
] );

This would register an Interface:

Interface NodeWithFeaturedPostsConnection {
  featuredPosts: UserToFeaturedPostConnection
}

And the return type of the featuredPosts field would be a connection that implements a more general connection:

type UserToFeatruedPostConnection implements UserToPostConnection {
  nodes: [ Post ]
  edges: [ UserToPostEdge ]
}

And UserToPostConnection (which already exists) implements ContentNodeConnection and Connection Interfaces.

Then I could create a fragment like so:

fragment FeaturedPosts on NodeWithFeaturedPostsConnection {
  featuredPosts {
    nodes {
      id
    }
  }
}

But the connection itself could also be a more generic fragment:

fragment UserPosts on UserToPostConnection {
  nodes {
    id
  }
}

fragment FeaturedPosts on NodeWithFeaturedPostsConnection {
  featuredPosts {
    ...UserPosts
  }
}

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically closed because it has not had recent activity. If you believe this issue is still valid, please open a new issue and mark this as a related issue.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically closed because it has not had recent activity. If you believe this issue is still valid, please open a new issue and mark this as a related issue.