Coverage increased (+0.08%) to 81.796% when pulling e96a0c5 on jasonbahl:feat/#1738-introduce-common-interfaces-2 into 6e63a79 on wp-graphql:develop.

Code Climate has analyzed commit e96a0c5 and detected 58 issues on this pull request.

Here's the issue category breakdown:

View more on Code Climate.

Really cool stuff. Do we want to keep the old (admittedly poorly-named) ContentRevisionUnion instead of ContentRevision for back-compat?

Really cool stuff. Do we want to keep the old (admittedly poorly-named) ContentRevisionUnion instead of ContentRevision for back-compat?

Ya, perhaps we keep the ContentRevisionUnion Type, and deprecate the revisions connections that resolve to it, then introduce a new field with a new name that resolves as a ContentNode connection. . .then remove the deprecated type and field later.

Thoughts on a new name for the field?

post.revisions @deprecated
post.contentRevisions?

There's only 2 places the ContentRevisionUnion was referenced:

RootQuery.revisions
User.revisions

I'm thinking we can deprecate these and introduce:

RootQuery.revisedNodes
User.revisedNodes

Not sure about revisedNodes, since semantically it implies nodes that have been revised and not individual revision snapshots of the particular node... But idk naming things is hard 😵‍💫

@justlevine ah! I remember why I had to make this change.

With the introduction of the Connection and Edge interfaces applied to all connections, we get errors like this now:

Interface field Connection.nodes expects type [Node!]! but UserToRevisionsConnection.nodes is type [ContentRevisionUnion!]!.

This is because all connections now expect to connect to a Node type, and a Union can't be a node type, because unions can't implement interfaces.

I think the best way forward is to make this "technically" breaking change.

For most use (99%+?) cases, it won't actually be a breaking change.

Because the previous connection returned a Union instead of an Interface, the only way to query was like so (using the ...on syntax for every single type)

{
  viewer {
    revisions {
      nodes {
        __typename
        ... on Post {
          id
          uri
          isRevision
        }
        ... on Page {
          id
          uri
          isRevision
        }
      }
    }
  }
  revisions {
    nodes {
      __typename
      ... on Post {
        id
        uri
        isRevision
      }
      ... on Page {
        id
        uri
        isRevision
      }
    }
  }
}

Since the new connection is an Interface, the same queries will still work.

v1.12.2

Here's the same query on WPGraphQL v1.12.2 (ContentRevisionUnion)

Current PR

And here's the same query with the changes from this PR applied

The only way it would cause a break would be if someone were querying like so:

{
  viewer {
    revisions {
      ... on UserToContentRevisionUnionConnection {
        nodes {
          __typename
          ... on Post {
            id
            uri
            isRevision
          }
          ... on Page {
            id
            uri
            isRevision
          }
        }
      }
    }
  }
  revisions {
    ... on RootQueryToContentRevisionUnionConnection {
      nodes {
        __typename
        ... on Post {
          id
          uri
          isRevision
        }
        ... on Page {
          id
          uri
          isRevision
        }
      }
    }
  }
}

Which seems highly unlikely, but something we can call out in the release notes.

fwiw, on a different note, working on this makes me want to re-visit #2355. I still don't quite now how to resolve that "problem" but it keeps haunting me.

I have several one-off utility post types that I don't want to be lumped together as "content" but they keep showing up in my contentNodes { ... } queries. lol

@justlevine we could potentially add something to the connection config for exluding the Connection and Edge interfaces too 🤔

like:

register_graphql_connection( [
  'fromType' => 'RootQuery',
  'toType'               => 'ContentRevisionUnion',
  ...
  'exclude_interfaces' => [ 'Connection' ],
  'exclude_edge_interfces' => [ 'Edge' ],
] );

🤔

I think it might be best to rip the bandaid off though, as the breaking change will likely not affect anyone, as explained above, and now we can ensure going forward that all connections properly lead to Nodes.