Add block nesting governance schema updates and docs #43801

alecgeatches · 2022-09-02T08:57:01Z

This PR contains the schema updates to theme.json used in the block nesting implementation in #43796. That PR allows for nesting blocks in theme.json like this:

// theme.json → settings
"blocks": {
    "core/child-block": {
        "color": {
            "text": false,
        },
    },
    "core/parent-block": {
        "core/child-block": {
            "color": {
                "text": true,
            },
        },
    },
},

With the setup above, a core/child-block will have color.text === true when nested in a core/parent-block. We've added this as a separate PR, since these schema changes are a bit more controversial and might be a different discussion.

What?

There are a handful of related changes bundled here:

Add schema validation in theme-schema.test.js based on existing block schema tests. The existing schema failed validation with Ajv's strict mode against draft-04, mostly due to some missing type identifiers and a handful of other small changes.
Fix the existing strict mode errors.
Update the schema to support nested block settings.
Update relevant docs related to the schema.

Why?

Block nesting support has been added to support the theme.json parsing and block editor changes in #43796.

While we were updating the schema, it seemed like a good idea to put the existing schemas/json/theme.json file under test to ensure nothing was getting broken. Since the existing schema did not pass Ajv's draft-04 validation, some minor changes were made to fix the errors. These are covered in the How section.

How

Validation changes

As discussed above, some minor changes were made to the original schema to make it validate correctly against draft-04. To view the original errors, check out the add/theme-schema-updates-and-docs branch of the related fork and use the 7bc6ee9 commit to view the original errors. Running the test gives this validation result:

Click to expand

$ npx wp-scripts test-unit-js --config test/unit/jest.config.js --no-cache theme-schema.test.js
 FAIL  test/integration/theme-schema.test.js
  ● theme.json schema › schema valid

    strict mode: property core/archives matches pattern ^[a-z][a-z0-9-]*/[a-z][a-z0-9-]*$ (use allowMatchingProperties)

      13 |
      14 |      test( 'schema valid', () => {
    > 15 |              const result = ajv.validate( themeSchema, {} ) || ajv.errors;
         |                                 ^
      16 |
      17 |              expect( result ).toBe( true );
      18 |      } );

      at checkStrictMode (node_modules/ajv/lib/compile/util.ts:211:28)
      at checkMatchingProperties (node_modules/ajv/lib/vocabularies/applicator/patternProperties.ts:54:26)
      at validatePatternProperties (node_modules/ajv/lib/vocabularies/applicator/patternProperties.ts:40:30)
      ...
      at Ajv.compile (node_modules/ajv/lib/core.ts:370:34)
      at Ajv.validate (node_modules/ajv/lib/core.ts:346:16)
      at Object.<anonymous> (test/integration/theme-schema.test.js:15:22)
          at runMicrotasks (<anonymous>)

  ● theme.json schema › schema valid

    expect(jest.fn()).not.toHaveWarned(expected)

    Expected mock function not to be called but it was called with:
    ["strict mode: missing type \"object\" for keyword \"properties\" at \"#/definitions/settingsPropertiesAppearanceTools\" (strictTypes)"]
    ["strict mode: missing type \"object\" for keyword \"properties\" at \"#/definitions/settingsPropertiesBorder\" (strictTypes)"]
    ["strict mode: missing type \"object\" for keyword \"properties\" at \"#/definitions/settingsPropertiesColor\" (strictTypes)"]
    ["strict mode: missing type \"object\" for keyword \"properties\" at \"#/definitions/settingsPropertiesLayout\" (strictTypes)"]
    ["strict mode: missing type \"object\" for keyword \"properties\" at \"#/definitions/settingsPropertiesSpacing\" (strictTypes)"]
    ["strict mode: missing type \"object\" for keyword \"properties\" at \"#/definitions/settingsPropertiesTypography\" (strictTypes)"]
    ["strict mode: use allowUnionTypes to allow union type keyword at \"#/definitions/settingsPropertiesTypography/properties/typography/properties/fontSizes/items/properties/fluid\" (strictTypes)"]
    ["strict mode: type \"integer\" not allowed by context \"string\" at \"#/definitions/settingsPropertiesTypography/properties/typography/properties/fontFamilies/items/properties/fontFace/items/properties/fontWeight/oneOf/1\" (strictTypes)"]
    ["strict mode: missing type \"object\" for keyword \"properties\" at \"#\" (strictTypes)"]
    ["strict mode: missing type \"object\" for keyword \"additionalProperties\" at \"#/properties/core~1archives\" (strictTypes)"]
    ["strict mode: missing type \"object\" for keyword \"properties\" at \"#/definitions/settingsPropertiesAppearanceTools\" (strictTypes)"]
    ["strict mode: missing type \"object\" for keyword \"properties\" at \"#/properties/core~1button/allOf/1\" (strictTypes)"]
    ["strict mode: missing type \"object\" for keyword \"properties\" at \"#/definitions/settingsPropertiesColor\" (strictTypes)"]
    ["strict mode: missing type \"object\" for keyword \"properties\" at \"#/definitions/settingsPropertiesLayout\" (strictTypes)"]
    ["strict mode: missing type \"object\" for keyword \"properties\" at \"#/definitions/settingsPropertiesSpacing\" (strictTypes)"]
    ["strict mode: missing type \"object\" for keyword \"properties\" at \"#/definitions/settingsPropertiesTypography\" (strictTypes)"]
    ["strict mode: use allowUnionTypes to allow union type keyword at \"#/definitions/settingsPropertiesTypography/properties/typography/properties/fontSizes/items/properties/fluid\" (strictTypes)"]
    ["strict mode: type \"integer\" not allowed by context \"string\" at \"#/definitions/settingsPropertiesTypography/properties/typography/properties/fontFamilies/items/properties/fontFace/items/properties/fontWeight/oneOf/1\" (strictTypes)"]

      30 |      function assertExpectedCalls() {
      31 |              if ( spy.assertionsNumber === 0 && spy.mock.calls.length > 0 ) {
    > 32 |                      expect( console ).not[ matcherName ]();
         |                      ^
      33 |              }
      34 |      }
      35 |

      at Object.assertExpectedCalls (packages/jest-console/src/index.js:32:4)
          at runMicrotasks (<anonymous>)
      at TestScheduler.scheduleTests (node_modules/@jest/core/build/TestScheduler.js:333:13)
      at runJest (node_modules/@jest/core/build/runJest.js:404:19)
      at _run10000 (node_modules/@jest/core/build/cli/index.js:320:7)
      at runCLI (node_modules/@jest/core/build/cli/index.js:173:3)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        2.093 s
Ran all test suites matching /theme-schema.test.js/i.

Those individual fixes are documented in these commits:

Fix missing property object types for errors like:

strict mode: missing type "object" for keyword "properties" at "#/definitions/settingsPropertiesAppearanceTools" (strictTypes)

Remove type conflict in fontWeight for this error:

strict mode: type "integer" not allowed by context "string" at "#/definitions/settingsPropertiesTypography/properties/typography/properties/fontFamilies/items/properties/fontFace/items/properties/fontWeight/oneOf/1" (strictTypes)

Refactor fontSizes.fluid into non-union type for this error:

strict mode: use allowUnionTypes to allow union type keyword at "#/definitions/settingsPropertiesTypography/properties/typography/properties/fontSizes/items/properties/fluid" (strictTypes)

Fix core/archives non-property block having additionalProperties: false for this error:

strict mode: missing type "object" for keyword "additionalProperties" at "#/properties/core/archives" (strictTypes)

Some additional fixes in Fix stylesProperties type and Fix styleProperties border direction properties that cropped up after the previous fixes, mostly addressing additional properties object types missing. This also fixes the border autogenerated documentation for top/bottom/left/right.

Nested styling changes

To support nested styling, some new definitions were added:

settingsBlocksPropertiesComplete now wraps a settingsBlocksProperties definition using the style mentioned in the current PR by @ajlende.
settingsNestedPropertiesComplete was added, which wraps settingsProperties and settingsBlocksProperties.
In settingsBlocksProperties, blocks which support nesting use settingsNestedPropertiesComplete, and all other core blocks default to using settingsProperties.

There is some duplication due to draft-04 compatibility. For example, the core/archives block settings definitions are repeated in these places:

settingsBlocksProperties (used by settingsBlocksPropertiesComplete and settingsNestedPropertiesComplete).
settingsBlocksPropertiesComplete (used by the top level settings.blocks definition)
settingsNestedPropertiesComplete (used by blocks that support nesting)

As far as we're aware, this is the most succinct way to represent these nested properties in draft-04. The previous version of this PR used draft-07 and propertyNames, but after discussion with @ajlende it's been rewritten to be draft-04 compatible.

Testing Instructions

To test theme validation, run:

npx wp-scripts test-unit-js --config test/unit/jest.config.js --no-cache theme-schema.test.js

Schema changes are manually testable by editing an existing theme.json file and setting the schema to the new theme.json schema in this branch:

{
	"$schema": "/<full-path-to>/gutenberg/schemas/json/theme.json",
	"version": 2,
	"settings": {
            /* ... */
        }
}

Screenshots or screencast

Here's a quick demo showing nested block settings working with the draft-07 schema with autocompletion in VS Code:

theme-json-block-nesting.mp4

github-actions · 2022-09-02T08:57:23Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @alecgeatches! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

ajlende

Thank you so much for fixing all of those issues that the schema has! And adding the schema validation test is another huge win!

We couldn't find a draft-04-compatible setup that would allow for this configuration. Instead, this uses draft-07's propertyNames feature workaround mentioned in the link above.

The example in that link can be achieved in draft-04, albeit not as cleanly as with draft-07's propertyNames.

Expand to reference the example from the link.

This fails because an object with both foo and bar doesn't pass both tests individually.

{
    "type": "object",
    "allOf": [
        { "$ref": "#/definitions/foo" },
        { "$ref": "#/definitions/bar" }
    ],
    "definitions": {
        "foo": {
            "properties": {
                "foo": { "type": "string" }
            },
            "additionalProperties": false
        },
        "bar": {
            "properties": {
                "bar": { "type": "number" }
            },
            "additionalProperties": false
        }
    }
}

In draft-07 the merging is done like this.

{
    "type": "object",
    "allOf": [
        { "$ref": "#/definitions/foo" },
        { "$ref": "#/definitions/bar" }
    ],
    "propertyNames": {
        "anyOf": [
            { "$ref": "#/definitions/fooNames" },
            { "$ref": "#/definitions/barNames" }
        ]
    },
    "definitions": {
        "foo": {
            "properties": {
                "foo": { "type": "string" }
            }
        },
        "fooNames": { "enum": ["foo"] },
        "bar": {
            "properties": {
                "bar": { "type": "number" }
            }
        },
        "barNames": { "enum": ["bar"] }
    }
}

In draft-04, you have to manually merge the property names like this.

{
    "type": "object",
    "allOf": [
        { "$ref": "#/definitions/foo" },
        { "$ref": "#/definitions/bar" },
        {
            "properties": {
                "foo": {},
                "bar": {}
            },
            "additionalProperties": false
        }
    ],
    "definitions": {
        "foo": {
            "properties": {
                "foo": { "type": "string" }
            }
        },
        "bar": {
            "properties": {
                "bar": { "type": "number" }
            }
        }
    }
}

In the theme.json schema, we use the terminology *Complete when a definition includes the empty properties and "additionalProperties": false. It makes for a long list of definitions when you need to add every combination manually, but it is possible with to add all of them.

{
    "type": "object",
    "properties": {
        "foo": { "$ref": "#/definitions/fooComplete" },
        "bar": { "$ref": "#/definitions/barComplete" },
        "foobar": { "$ref": "#/definitions/foobarComplete" }
    },
    "additionalProperties": false,
    "definitions": {
        "foo": {
            "properties": {
                "foo": { "type": "string" }
            }
        },
        "fooComplete": {
            "allOf": [
                { "$ref": "#/definitions/foo" },
                {
                    "properties": { "foo": {} },
                    "additionalProperties": false
                }
            ]
        },
        "bar": {
            "properties": {
                "bar": { "type": "number" }
            }
        },
        "barComplete": {
            "allOf": [
                { "$ref": "#/definitions/bar" },
                {
                    "properties": { "bar": {} },
                    "additionalProperties": false
                }
            ]
        },
        "foobar": {
            "allOf": [
                { "$ref": "#/definitions/foo" },
                { "$ref": "#/definitions/bar" }
            ]
        },
        "foobarComplete": {
            "allOf": [
                { "$ref": "#/definitions/foobar" },
                {
                    "properties": {
                        "foo": {},
                        "bar": {}
                    },
                    "additionalProperties": false
                }
            ]
        }
    }
}

Unfortunately, with this method, all places that merge properties need to update their own list of properties when a new property is added to any of the children—draft-07's propertyNames would avoid the problem.

One more thing to consider is that the WordPress core JSON Schema validator used by the REST API only supports draft-04. If we ever wanted to utilize that for theme.json (we're currently doing custom validation), we'd also want to stick with draft-04.

See if you can refactor it to draft-04 using the method shown above. I personally think that the benefits of sticking to draft-04 are worth the maintenance burden that propertyNames helps with.

ajlende · 2022-09-02T17:25:33Z

test/integration/theme-schema.test.js

+	} );
+
+	test( 'schema valid', () => {
+		const result = ajv.validate( themeSchema, baseThemeJson ) || ajv.errors;


First off, thank you for adding tests here! Based on the number of things you've had to fix, this is sorely needed.

I think we could validate the schema directly rather than indirectly by validating data. I think you should be able to do something like this with AJV.

Suggested change

const result = ajv.validate( themeSchema, baseThemeJson ) || ajv.errors;

const result = ajv.validate(

'http://json-schema.org/draft-07/schema#',

themeSchema

) || ajv.errors;

ajlende · 2022-09-02T20:00:14Z

schemas/json/theme.json

 		},
 		"settingsBlocksPropertiesComplete": {
 			"type": "object",
 			"properties": {
 				"core/archives": {
-					"description": "Archive block. Display a monthly archive of your posts. This block has no block-level settings",
-					"additionalProperties": false


The archives block does not have any block-level settings, so we should probably update this to add "type": "object" rather than removing "additionalProperties": false

Right, that makes sense to me!

ajlende · 2022-09-02T20:05:29Z

schemas/json/theme.json

 				},
 				"core/audio": {
-					"$ref": "#/definitions/settingsPropertiesComplete"
+					"$ref": "#/definitions/settingsPropertiesNested"


If a block doesn't support inner blocks, should we allow nested properties?

We're avoiding the specific settings that are available to each block because blocks are getting new supports pretty often and there's a high maintenance overhead for keeping the specifics up to date, but blocks updating to support nesting happens pretty rarely, so I don't think it would be too big of a problem to use settingsPropertiesComplete for blocks that don't have inner blocks.

That's a great idea - limiting which blocks allow nesting via the schema will also make the intellisense a lot more helpful.

alecgeatches · 2022-09-05T06:36:12Z

@ajlende Thank you for all of the great feedback! I've applied your guidance using *Complete definitions and figured out how to make these changes draft-04 compatible. Please see the updated issue description which has removed all of the draft-07 discussion and should match the behavior of the current code. Here were the main changes made to address your comments:

Added new *Complete properties to support nesting, covered by the new Nested styling changes section in the issue description.

Per your suggestion, changed the schema to only have nested settings for blocks that support inner blocks in 7749202. Here's the heuristic used to determine blocks with nesting:

$ cd gutenberg

# Find files recursively in packages/block-library/src that contain inner block usage
# Pull out the block name from the path and add "core/" to the front
# Remove duplicates and sort the results

$ grep -rile '<InnerBlocks\|innerBlocksProps' packages/block-library/src | awk -F '/' '{print "core/"$4}' | uniq | sort
core/block
core/buttons
core/column
core/columns
core/comment-template
core/comments
core/comments-pagination
core/cover
core/gallery
core/group
core/home-link
core/list
core/list-item
core/media-text
core/navigation
core/navigation-link
core/navigation-submenu
core/post-comment
core/post-content
core/post-template
core/query
core/query-no-results
core/query-pagination
core/quote
core/social-links
core/template-part

These probably need to be tweaked and could use a second look. The video at the bottom of the issue description has also been updated to show the new behavior for nestable/unnestable blocks in VS Code.

The validation code no longer uses an object to validate the schema. This ended up being trickier than expected. Your suggested code change using validate ran successfully, but did not report and strict errors on theme.json's schema. For example, I tried adding invalid properties like "test": "abc" to the schema which should fail Ajv's strictSchema, but no errors were reported. The only way I could get the meta-schema to be validated was by validating an actual data structure or using .compile():

gutenberg/test/integration/theme-schema.test.js

Line 19 in 7749202

const result = ajv.compile( themeSchema );

It's unclear from Ajv's documentation why .validate('.../draft-04#', $themeSchema) or .validate($themeSchema) weren't reporting errors. The best I could find was the documentation for compile() which mentions the meta-schema:

The schema passed to this method will be validated against meta-schema unless validateSchema option is false. If schema is invalid, an error will be thrown. See options.

It seems like the meta-schema is only validated during compilation, so the code has been changed to use that instead.

luisherranz · 2022-09-06T07:04:07Z

Wouldn't it make sense to put the definition of blocks inside "blocks", just like in the root level?

{
  "color": {
    "text": true
  },
  "blocks": {
    "core/child-block": {
      "color": {
        "text": false
      }
    },
    "core/parent-block": {
      "color": {
        "text": false
      },
      "blocks": {
        "core/child-block": {
          "color": {
            "text": true
          }
        }
      }
    }
  }
}

If we skip "blocks", people would need to learn that they need to use "blocks" at the root level but avoid it when they are nested.

And also, wouldn't that make the schema definition easier (as you just have to repeat whatever schema "blocks" has in the root)?

alecgeatches · 2022-09-07T02:45:42Z

@luisherranz Thank you for the feedback! @ingeniumed and I have been discussing this. We agree it would definitely make the schema changes easier, but we’re not sure if nesting blocks is a cause for confusion:

If we skip “blocks”, people would need to learn that they need to use “blocks” at the root level but avoid it when they are nested.

From the perspective of the designer/developer in theme.json, we don’t think this would be problem. Block settings will go in settings.blocks, and all of the properties in there (nested or not) are block settings. This also sort of works like CSS/SASS in the current format - you just add a child “selector” block in a parent with new settings and you’re good to go. That seems conceptually easier to understand to us than repeating the blocks property.

Your suggested approach would be good for simplifying the schema changes by namespacing the new changes to a blocks property, but will add some more layers and we’re not sure if it’ll be an improvement. If we change the schema here we’d also need to make changes to theme.json parsing/block editor consumption in #43796 as well, so we want to make sure.

@luisherranz We’d love to hear any follow-up thoughts on this or if anyone else can add in as well. Thank you!

We’d also be interested in @dabowman’s opinion here on @luisherranz schema changes above from the designer perspective.

luisherranz · 2022-09-07T07:10:22Z

Sure. Let's bring more opinions: cc @mtias, @mcsf, @youknowriad, @oandregal, @jorgefilipecosta.

alecgeatches · 2022-09-19T04:30:28Z

Given our chat with the Gutenberg IO team and @mcsf's comment here with a different approach, we're likely not going to need the schema changes for nesting. However, since the original test changes and schema fixes may provide some value, we're going to split that off into a different PR. We'll plan to keep this PR around for a bit until we're sure #43796 isn't going to be involved.

Thank you!

alecgeatches · 2022-09-19T05:20:42Z

Please see #44252 for just the test changes to the existing schema. Thank you!

alecgeatches · 2022-10-19T03:25:32Z

Closing this issue as the related nesting governance in #43796 has been closed, and the unit test changes from this PR were merged separately in #44252. @ingeniumed has created another (smaller) PR in #45089 to address governance within a plugin. Thank you!

alecgeatches and others added 11 commits September 2, 2022 16:32

Add failing test for valid theme.json schema

7bc6ee9

Fix missing property object types

ce7e75a

Remove type conflict in fontWeight

66bf0b8

Refactor fontSizes.fluid into non-union type

5700764

Fix core/archives non-property block having additionalProperties: false

6095a7a

Fix stylesProperties type

cd3e2b3

Fix styleProperties border direction properties

441f338

Add required schema, required strict skips to test

b49655d

Run docs:build for theme.json schema changes

f89f936

Upgrade theme.json schema to draft-07, add nesting

fbb57ec

Add an example of a nested block's settings in the theme.json

f60681f

alecgeatches requested review from ajlende, ajitbohra, ryanwelcher, juanmaguitar and fabiankaegy as code owners September 2, 2022 08:57

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Sep 2, 2022

alecgeatches mentioned this pull request Sep 2, 2022

Add block nesting governance for theme.json #43796

Closed

annezazu added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Sep 2, 2022

ajlende reviewed Sep 2, 2022

View reviewed changes

alecgeatches added 6 commits September 5, 2022 10:19

Fix test to directly validate schema against spec

ce20a4c

Fix core/archives block to have object type

cea1e32

Convert to draft-04, use *Complete properties

1b850f9

Update schema test to use draft-04 and test compilation

8d223d0

Change blocks to use regular properties by default

c6e6bd2

Update blocks with inner blocks to use nested properties

7749202

alecgeatches mentioned this pull request Sep 19, 2022

Add theme.json schema tests #44252

Merged

alecgeatches closed this Oct 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add block nesting governance schema updates and docs #43801

Add block nesting governance schema updates and docs #43801

alecgeatches commented Sep 2, 2022 •

edited

Loading

github-actions bot commented Sep 2, 2022

ajlende left a comment •

edited

Loading

ajlende Sep 2, 2022

ajlende Sep 2, 2022

alecgeatches Sep 5, 2022

ajlende Sep 2, 2022

alecgeatches Sep 5, 2022

alecgeatches commented Sep 5, 2022 •

edited

Loading

luisherranz commented Sep 6, 2022 •

edited

Loading

alecgeatches commented Sep 7, 2022 •

edited

Loading

luisherranz commented Sep 7, 2022

alecgeatches commented Sep 19, 2022 •

edited

Loading

alecgeatches commented Sep 19, 2022

alecgeatches commented Oct 19, 2022

Add block nesting governance schema updates and docs #43801

Add block nesting governance schema updates and docs #43801

Conversation

alecgeatches commented Sep 2, 2022 • edited Loading

What?

Why?

How

Validation changes

Nested styling changes

Testing Instructions

Screenshots or screencast

github-actions bot commented Sep 2, 2022

ajlende left a comment • edited Loading

Choose a reason for hiding this comment

ajlende Sep 2, 2022

Choose a reason for hiding this comment

ajlende Sep 2, 2022

Choose a reason for hiding this comment

alecgeatches Sep 5, 2022

Choose a reason for hiding this comment

ajlende Sep 2, 2022

Choose a reason for hiding this comment

alecgeatches Sep 5, 2022

Choose a reason for hiding this comment

alecgeatches commented Sep 5, 2022 • edited Loading

luisherranz commented Sep 6, 2022 • edited Loading

alecgeatches commented Sep 7, 2022 • edited Loading

luisherranz commented Sep 7, 2022

alecgeatches commented Sep 19, 2022 • edited Loading

alecgeatches commented Sep 19, 2022

alecgeatches commented Oct 19, 2022

alecgeatches commented Sep 2, 2022 •

edited

Loading

ajlende left a comment •

edited

Loading

alecgeatches commented Sep 5, 2022 •

edited

Loading

luisherranz commented Sep 6, 2022 •

edited

Loading

alecgeatches commented Sep 7, 2022 •

edited

Loading

alecgeatches commented Sep 19, 2022 •

edited

Loading