General Feedback on Schema Design

[jasonbahl]

This is some general feedback after initial tinkering with the plugin.

First off, good stuff! This is working similar to what I had in mind when I began work on WPGraphQL Block Editor. 🙌🏻

I have some feedback I think we should consider, ideally before we get too many users as they would be breaking changes.

Rename ContentBlock Interface

Not necessarily a hill I will die on, but I think Content usually has connotations with things like blog posts, but nav menus (in my mind) are not the same as "content".

I had selected "EditorBlock" as the name of the interface because each block is a block of the block editor. It might be a ContentBlock, or it might be a menu block or a layout block, etc.

I think we can do users (and ourselves) a service by being more clear with the naming.

Maybe something like:

• EditorBlock: All blocks that can be used within the Block Editor
• ContentBlock (implements EditorBlock): Blocks used to manage content
• LayoutBlock (implements EditorBlock): Blocks used to manage layout
• NavMenuBlock(?) (Implements EditorBlock): Blocks used by the block editor when managing nav menus

By having a generic (EditorBlock) Interface, and then more specific Interfaces (Content, Layout, etc) we can better use the Schema as documentation, and provide better experiences for our users.

We can even get more specific like:

• PostContentBlock (Implements ContentBlock, EditorBlock): Blocks used to manage content on the "Post" type
• PageContentBlock (Implements ContentBlock, EditorBlock): Blocks used to manage content on the "Page" type

etc.

This way, we can allow the Schema to better differentiate for consumers what blocks they can expect.

For example, if I register a custom block only on the Car post type, It shouldn't show up as a possible block on the Page and Post post types, because we know the block will never be available on those types.

So, if our schema for posts was:

# A block used by the block editor
Interface EditorBlock {
  name: Name of the block
  clientId: Non-persisted ID used to map blocks by the client application
  clientParentId: Non-persisted ID of the parent block used to map hierarchical relationships in the client application
}

# Editor Block used to manage content
Interface ContentBlock implements EditorBlock {
  name: Name of the block
  someFieldThatExistsForContentBlocksButNotOtherBlocksLikeNavMenuBlocks: Just an example
}

Interface ContentNode implements NodeWithContentBlocks {
  ...
  # List of blocks used to produce the content of the ContentNode (instead of returning _all_ EditorBlocks, it would return _just_ ContentBlocks, a subset of blocks used _just_ for Content)
  contentBlocks: [ContentBlock]
}

# A ContentBlock that is associated with the Post post type (i.e.https://www.designbombs.com/registering-gutenberg-blocks-for-custom-post-type/)
Interface PostContentBlock implements ContentBlock, EditorBlock {
  name: Name of the block
}

# We'd use register_graphql_interfaces_to_types() to accomplish this
type Post implements NodeWithPostContentBlocks {
  # the Post type will return a list of PostContentBlock (not a list of ContentBlock or EditorBlock) as this will most accurately describe in the schema what's actually possible to be returned. We _know_ that the post.contentBlocks will not return blocks registered only to the "Car" post type, so we should be able to properly express this in the Schema.
  contentBlocks: [PostContentBlock]
}

Rename nodeId field

Unfortunately, Blocks are not actually "nodes" as they cannot be accessed individually via node( id: "..." ) queries like other nodes.

I recommend we change this field name to clientId rather than nodId as these are not nodes and the field name could cause confusion.

Probably should also change parentId to clientParentId or parentClientId as well to clarify intent of the usage of these fields.

NOTE: If we're able to come up with a reliable solution for persisting block IDs (uniqueId persisted with blocks in the database, we might be able to make blocks actual nodes, which would be rad, but there's a lot of challenges to this we'd need to figure out still).

[theodesp]

Thanks @jasonbahl. We've created some tickets for review. We will have to meet to discuss the advanced schema you are proposing as I'm not confident on how to implement this one.

[jasonbahl]

@theodesp sounds good! Happy to meet and chat about it!

[justlevine]

@theodesp if 'blessed' tasks are added to the repo issues, I'm happy to volunteer some PRs.

[justlevine]

I'm curious how this relates to #25 , as that PR didn't add the EditorBlock as a parent interface but instead replaced the ContentBlock interface entirely and even changed the field name to editorBlocks and the corresponding NodeWith interface.

This seems like a big step backwards from @jasonbahl 's suggestion (or rather, how I understood it).

Since GraphQL is supposed to be self-documenting, an Interface name works best when it describes its shape/purpose in relation to others that share the same tree. So EditorBlock defines the generic shape (personally I'd have gone even broader with Block), and then the child interface names describe how/why those shapes differ.

(This early on in schema exploration, it might not be so evident what the differences are / if we need specific ContentBlock or LayoutBlock types. A clearer example is StaticBlock and DynamicBlock; the latter e.g. has both raw and rendered versions of the output.).

GraphQL fields however supply the context and the usage for the defined data shape. In this light editorBlocks: EditorBlock[] (or an inheriting type) is a list of... what? All blocks? Blocks used to render the Block Editor? It even makes you start to doubt what EditorBlock as a type is supposed to mean (why I prefer the generic Block). contentBlocks (the blocks used to generate the content field), templateBlocks, layoutBlocks (the blocks used to render the page content around the contentField), and even blocks:Block[]` (the entire block markup for the URI) are significantly more semantic and self documenting.

Tl;Dr

In the short term, I highly recommend reverting parts of #25, specifically restoring the contentBlocks field and its parent NodeWithContentBlocks interface). If there's no defining difference at this point between an EditorBlock and a ContentBlock, then that change is fine, but otherwise ContentBlock should be restored as well, with it's non-defining fields being what remains on EditorBlock.

More broadly, I suggest mapping out the possible difference in data shapes (type names) and contexts (field names), so the naming patterns are more intuitive and less prone to requiring future breaking changes.

[theodesp]

Hey @justlevine . Thanks for the feedback. My understanding is that:

EditorBlock: All blocks that can be used within the Block Editor

is the base interface for all other subtypes of blocks (ContentBlock, LayoutBlock, etc) hence I reverted back the original intent of this interface name.

However I should mention here that we haven't decided to go forward with the more specific Interfaces (Content, Layout, etc) and the reason is that we need to think about how those blocks are going to be queried in a headless site.

I can imagine a chaotic way of doing it because the developer won't be able to determine which block would be available as Content, Layout, NavMenuBlock since that information would be coded in the schema.

If on the other hand, one decides to use Faust and pass on all the block fragments in order then they could get errors for using block fragments not available in the GraphQL schema. Which ones to use for which page/layout/navbar?

Oh I know let's use three different fragment packages of blocks.

const pageBlockFragments = gql`...`; // collect fragments for page
const layoutBlockFragments = gql`...`; // collect fragments for layout
const navbarBlockFragments = gql`...`; // collect fragments for nav

Component.query = gql`
...
${pageBlockFragments}
${layoutBlockFragments}
${navbarBlockFragments}
editorBlocks {
        name
        __typename
        renderedHtml
        id: nodeId
        parentId
        ...
      }

Oh no. what happens if some blocks are common between them? Now if you add duplicate fragments you will get an error in the GraphQL request since you cannot re-declare them.

Fragment management suddenly could become harder and less convenient so that's why I'm only restoring the original intent on using the EditorBlock per WPGraphQL Block Editor as the base interface for now and gradually refine it until we find a workable solution.

[justlevine]

Thanks for the detailed reply @theodesp !

Appreciate the clarification that the EditorBlock interface is intended to be the parent type. From your response, I still dont understand why the field name was changed to editorBlock (along with the corresponding NodeWith* interface) - my more immediate concern.

To me it seems the confusion you're worried about is a direct result of this change: using a specifier ( editor block) to describe both something vague (anything shaped like Block data) and something else that has a conflicting specific meaning ( the editor blocks related to the post's content.

When faced with post.editorBlock, a user indeed doesn't immediately know that the field only contains block data parsed from the content field. Does this include the other block data that traditional WordPress outputs (e.g. the template wrapper or resolved block parts)? We know it doesnt, but the name itself is ambiguous and as you demonstrated with your query example, makes it really hard for us to scale up the schema without causing even more semantic confusion.

Now take this hypothetical query:

fragment BlockData on EditorBlock {
  name
  __typename
  id: nodeId
  parentId
  # We dont know if 'LayoutBlock', 'NavBlock' etc have semantic meaning yet, so using these for example's sake.
  ... on StaticEditorBlock {
      renderedHtml
  }
  ... on DynamicEditorBlock {
    ... BlockDataAttributesFragment
  }
}

query GetPosts( $uri: String! ) {
  nodeByUri( uri: $uri ) {
    ... on NodeWithTitle { # existing
      title
    }
    ... on NodeWithContent { #existing
      content
    }
    ... on NodeWithEditorBlocks {
      # Both the field and the interface are confusing to the user. Why doesnt the field include all editor blocks, and why does the interface only match WP_Post objects?
      editorBlocks {
        ...BlockData
     }
    ... on NodeWithContentBlocks {
      # This clearly tells the user what data is epected.
      contentBlocks {
        ...BlockData
      }

    # Above were the issues with the current naming.
    # Now lets theorycraft different possibilitiesto see the impact on extensibility.
    ... on Page {
      content
      contentBlocks {
        ...BlockData
      }
      anyOtherGroupOfBlocks { # has immediate semantic meaning because it doesnt conflict with a vague 'editorBlocks' field
        ...BlockData
      }
      template {
        blocks( format:RENDERED ) { # implies *all* blocks for the page. Could be editorBlocks to mirror your choice of Interface name.
          ...BlockData
        }
        layoutBlocks { # implies the blocks that wrap the content, i.e. Header/Footer.
          ...BlockData
        }
        activeTemplatePart( name: "navigation" ) {
          editorBlocks { # `why are these 'editor' blocks? Is it because theyre raw, and there's a separate frontendBlocks that handles dynamic data? 
            ...BlockData
          }
      }
      templatePart( name: HEADER ) {
        contentBlocks { # Even here this is more semantic than `editorBlocks`.
          ...BlockData
        }
      }
    }
  }
}

(I gave a bunch of various patterns; not advocating here for any specific one). In all of them, editorBlocks - at least as a field name - is both more confusing in and of itself and makes pretty much any other schema usage of blocks more confusing as well.

[theodesp]

Hey @justlevine since currently we are limiting the types of blocks registered using this function here that filters the blocks registered for only the post_type_supports( $block_editor_post_type->name, 'editor' ) helper. so my understanding again is that we would use only those as a base list of blocks hence the interface NodeWithEditorBlocks.

However would changing editorBlocks-> blocks would make more sense here since we do not care about the origin?

For example here is the comparison with wp-graphql-gutenberg plugin that filters the same list of blocks using get_post_types_by_support( 'editor' )

https://github.com/pristas-peter/wp-graphql-gutenberg/blob/bacabca662698871ecc8b297c7af3b5b95d431c1/src/Blocks/Utils.php#L23-L32

{
  nodeByUri(uri: "/events/this-is-new-post") {
    ...on BlockEditorContentNode {
      blocks {
        name
      }
    }
    ...on NodeWithEditorBlocks {
      editorBlocks {
        name
      }
    }
  }
}

The BlockEditorContentNode filters the list of blocks only available in the editor similar to what this plugin does. There is no interface the returns the list of all blocks using this plugin so we stick with that approach for now.

But again since editorBlocks is a field you can definitely use an alias so that it fits semantically for your use case:

{
  nodeByUri(uri: "/events/this-is-new-post") {
    ...on BlockEditorContentNode {
      blocks {
        name
      }
    }
    ...on NodeWithEditorBlocks {
      contentBlocks: editorBlocks {
        name
      }
    }
     ...on NodeWithEditorBlocks {
      blocks: editorBlocks {
        name
      }
    }
  }
}

[justlevine]

@theodesp your response seems to strengthen my argument even further, as you're acknowledging that even currently (meaning before accounting for future schema evolution) the semantic meaning of NodeWithEditorBlocks and editorBlocks does not match their user-facing function.

Ultimately it's your project. I only wanted to bring attention that despite your comment, the changes in #25 are actually a step back from (how I understood) @jasonbahl 's suggestions.

Since the changes in #25 seems to be an intentional decision and not just a misunderstanding of this ticket, I'll shut up now and let you get back to work 😎

[jasonbahl]

I think @justlevine understands the spirit of my suggestions well.

I believe we will benefit by having various interfaces that have meaning and can be used in different places to convey meaning.

The reason I suggested EditorBlock instead of Block is that it felt slightly less prone to Type conflicts in the schema.

The word Block could be used in different contexts unrelated to Gutenberg. The primary definitions of the word "Block" have nothing to do with Gutenberg or WordPress...

...and I could see some sites having a "Block" post type to represent things unrelated to Gutenberg.

I suggested EditorBlock to reduce the chances of conflict with other possible uses of the word "Block", as it seemed to be a bit more descriptive that it's a Block used with the Block Editor. There could easily be a better name, and possibly it is simply "Block". I'm open to that, and then any site that does have a Post Type or Taxonomy called block to store information about:

would be required to chose a different graphql_single_name for their post type / taxonomy 😆.

Regardless, the intent was to introduce a common Interface that defined the fields that all blocks will have in any context blocks could be used. . .which could be the content editor, nav menus, comments (btw, I was replying to some support threads on WordPress.org and noticed it's now using blocks in comments!), etc.

(For simplicity sake, I'll stick with "EditorBlock" as the Interface that means "Any block used with the block Editor, regardless of context". )

The idea was that for any specific context there would be another interface representing that specific contextual use of blocks:

• Interface ContentEditorBlock implements EditorBlock
• Interface NavMenuBlock implements EditorBlock
• Interface LayoutBlock implements EditorBlock
• Interface TemplateBlock implements EditorBlock
• Interface CommentEditorBlock implements EditorBlock
• Interface SomeNewFutureContextForBlocks implements EditorBlock

Then, when it comes to specific blocks, they should implement the interfaces that are relevant.

For example, if we had a SiteHeader block that was only intended to be used in full site editing as a block in that context, it could be:

type SiteHeader implements TemplateBlock {
  ...
}

Then, wherever we have fields that return a list of TemplateBlock, we would know that we could query for SiteHeader, but in a context where we have a field that returns a list of ContentBlock we would know that SiteHeader will never be a possible block to be returned from that field.

Same goes with the inverse.

If we know that a Paragraph block is intended to only be used within specific contexts (say, the "Content Editor" and "Comment Editor") we can describe that like so:

type CoreParagraph implements ContentEditorBlock && CommentEditorBlock {
  ...
}

Now, the schema will show the CoreParagraph block as a possible type when I query for a field that returns a list of ContentBlock or a list of CommentEditorBlock, but that field will not show SiteHeader as a possible block.

So, EditorBlock, in my mind, was the simplest Interface that should define all common fields of blocks used in any context (Content Editor, Nav Menu, Comments, etc)

Then, we would have more specific interfaces for those more specific contexts. Sometimes they would introduce new fields, sometimes they wouldn't but would just introduce more semantic meaning.

A CommentEditorBlock might even have the exact same fields as the EditorBlock, but it would convey that it's a bock used in the "comment editor".

Then the fields that expose the list of blocks would be able to expose a more narrow list of blocks.

Exposing ALL blocks in every field that returns blocks is misleading to the client developer trying to build something.

We know for a fact that we will never return a SiteHeader block in the Post.contentBlocks, and we know we will never expose a CoreParagraph when querying for RootQuery.templateBlocks (if that were a field, for example).

The schema should reflect what's possible, and we already know this isn't possible, but the Schema is misleading and saying it is possible.

That was my intent with the post-type specific interfaces as well.

If we know that a specific Block has been registered to only one specific post type, why would the Schema show it as a possible block to be returned on other post types?

As a client developer trying to make sense of the schema, it now makes no sense.

I want to know what's actually possible with the Schema, and right now the Schema is showing a lot of things that are impossible.

---

If on the other hand, one decides to use Faust and pass on all the block fragments in order then they could get errors for using block fragments not available in the GraphQL schema. Which ones to use for which page/layout/navbar?

Oh I know let's use three different fragment packages of blocks.

const pageBlockFragments = gql`...`; // collect fragments for page
const layoutBlockFragments = gql`...`; // collect fragments for layout
const navbarBlockFragments = gql`...`; // collect fragments for nav

Component.query = gql`
...
${pageBlockFragments}
${layoutBlockFragments}
${navbarBlockFragments}
editorBlocks {
        name
        __typename
        renderedHtml
        id: nodeId
        parentId
        ...
      }

Oh no. what happens if some blocks are common between them? Now if you add duplicate fragments you will get an error in the GraphQL request since you cannot re-declare them.

^ I think this feels solveable with a de-dupe method of some sort in Faust.

Something to the tune of:

const pageBlockFragments = gql`...`; // collect fragments for page
const layoutBlockFragments = gql`...`; // collect fragments for layout
const navbarBlockFragments = gql`...`; // collect fragments for nav

// a function that dedupes any fragments, should there be any duplicates
// and maybe does some other useful things. . .not sure what, 
// but I'm sure there will be something else some day we may need / want to centralize
const fragments = faustFragments([
  pageBlockFragments,
  layoutBlockFragments,
  navbarBlockFragments
]);

Component.query = gql`
...
${fragments}
editorBlocks {
        name
        __typename
        renderedHtml
        id: nodeId
        parentId
        ...
      }

I think one of the benefits of using Fragments / GraphQL is being able to ask for specifically what you need anyway, so I'm not sure using generic packages that define 100 fragments for blocks you have no intention of using, or that the field will never return is the way to go anyway.

Lets say I had a post type that I limited to ONLY have the CoreParagraph and CoreImage block.

Why would I want a query that looks like this:

{
  customPostTypeThatOnlyHasParagraphAndImage( id: "asdfasd" ) {
    id
    title
    contentBlocks { 
      ...FaustBlocks 
    }
  }
}

fragment FaustBlocks on EditorBlock {
  name
  renderedHtml
  ...on CoreQuote {...}
  ...on CoreHeading {...}
  ...on SiteHeader {...}
  ...on CoreColumn {...}
  ...on CoreOembed {...}
  ...on CoreCategories {...}
  ...on CoreWidgetGroup{...}
  ...on CoreArchives{...}
  ...on CoreAvatar{...}
  ...on CoreCalendar{...}
  ...on CoreComments{...}
}

This query makes no sense, in my opinion.

But, if the Post Type had a specific Interface that blocks available on that post type could implement, it would be much more useful to be able to construct queries.

If I were to have an interface like so:

interface PostTypeThatOnlyHasParagraphAndImageContentEditorBlock implements ContentEditorBlock && EditorBlock {
 ...
}

Then, any block that's allowed to be queried on that post type could implement PostTypeThatOnlyHasParagraphAndImageContentEditorBlock

i.e.:

type CoreParagraph implements ContentEditorBlock && EditorBlock && PostTypeThatOnlyHasParagraphAndImageContentEditorBlock { ... }

type CoreImage implements ContentEditorBlock && EditorBlock && PostTypeThatOnlyHasParagraphAndImageContentEditorBlock

Now, if the schema for the Post Type (PostTypeThatOnlyHasParagraphAndImage) looked like so:

type PostTypeThatOnlyHasParagraphAndImage implements ContentNode && Node && NodeWithTitle && ... {
   id: ID!
   title: String
   ...
   contentBlocks: [PostTypeThatOnlyHasParagraphAndImageContentEditorBlock]
}

Then when I looked at GraphiQL, I would accurately see that the contentBlocks when querying that type of node will only return CoreImage and CoreParagraph as possible types.

I don't have to write queries that need to anticipate every possible block registered, just the blocks that are actually possible on that post type.

I could query like this, instead:

{
  customPostTypeThatOnlyHasParagraphAndImage( id: "asdfasd" ) {
    id
    title
    contentBlocks { 
      ...on CoreParagraph {}
      ...on CoreaImage {}
    }
  }
}

This is much easier to debug as I know that this query is asking specifically for those 2 blocks (not the entire block library), and the Schema is telling me exactly what blocks I can expect for customPostTypeThatOnlyHasParagraphAndImage.contentBlocks, the 2 blocks that are actually possible to be returned.

[theodesp]

I'm closing this one. In the future we would prefer to have a more specific feature request format when opening PRs so that we can address them promptly.

[jasonbahl]

I'm closing this one. In the future we would prefer to have a more specific feature request format when opening PRs so that we can address them promptly.

@theodesp how would you have liked this to be done more specifically?

I feel like I gave pretty good detail in the original issue and in the follow-up discussion.