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

Add block binding support #74

Merged
merged 20 commits into from
Aug 30, 2024
Merged

Add block binding support #74

merged 20 commits into from
Aug 30, 2024

Conversation

chriszarate
Copy link
Member

@chriszarate chriszarate commented Aug 23, 2024

Description

This PR adds support for block bindings, introduced in WordPress 6.5.

The changes here are relatively small, although they are accompanied by some test improvements and cleanup, so the diff is a bit larger than it otherwise would be. Breakdown of commits:

New functionality

  • Update parser to leverage core block rendering: This is the most impactful commit:
    • In order to support block binding without copying many lines of code from core, we create a block instance and render it (including all of its inner blocks), then access the computed attributes and inner HTML. This lets Core do the heavy lifting.
    • Then, we walk the tree and apply our logic to extract sourced attributes.

Test improvements

No test data was changed in this PR (only ordering).

Additional test cases

@chriszarate chriszarate added the enhancement New feature or request label Aug 23, 2024
@chriszarate chriszarate requested a review from a team as a code owner August 23, 2024 21:24
alecgeatches
alecgeatches previously approved these changes Aug 27, 2024
Copy link
Contributor

@alecgeatches alecgeatches left a comment

Choose a reason for hiding this comment

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

I think this looks and works really well! I especially like the improvements to tests.

There's quite a bit that's changed in the core block rendering implementation, but with our existing test suite and the new ones you've added I'm confident that we're not breaking anything here.

I added some minor comments about comments, but the functionality looks great to me. I'd also like @ingeniumed to have a look soon as well.

@alecgeatches
Copy link
Contributor

@chriszarate The discussion in #74 (comment) made me think about a potential version bump to v2, as in vip-block-data-api/v2/.... We recently did a similar thing for the GraphQL endpoint with blockDataV2: #72.

Are there any changes in this PR or #75 that would justify a version upgrade? It seems like we're mostly adding more data to the response with the original binding data + actual computed binding results, which isn't necessarily a breaking change but could be. What do you think? This would entail some more work to duplicate some tests and separate a second parser class, but it may be an easier path and allow some other minor upgrades like we discussed.

@chriszarate
Copy link
Member Author

Are there any changes in this PR or #75 that would justify a version upgrade?

There aren't any breaking changes to the response shape—no new properties or changes to existing ones. The functional changes (supporting block bindings and synced patterns) reflect the current approach and behavior of the plugin, and just enhance the existing response.

Anything can be a surprise of course, but let's use some hypotheticals to guide us. All of these assume we release these two PRs and don't bump to v2:

Block bindings

The effect of block bindings in this plugin is potentially updated string attribute values for specific core blocks when block bindings are in use. There is currently no UI for block bindings in the block editor and very little adoption, so anyone using them must write and deploy code to provide them. If a Block Data API user also uses block bindings, they are certainly aware that they are not supported—and probably frustrated.

  1. I am a Block Data API consumer and I don't use block bindings. Upgrading has no effect.
  2. I am a Block Data API consumer and I use block bindings. I created my own implementation to resolve the bindings using the returned metadata attributes. Upgrading has no effect since my implementation governs and overrides. I can remove my implementation at any time of my choosing.
  3. I am a Block Data API consumer and I use block bindings. The returned data is incorrect, but I have decided to use it anyway. Upgrading has an effect by providing the correct data, and I consider this a breaking change.

Scenarios 1 and 2 are not breaking changes. A breaking change via scenario 3 is possible, but I struggle to imagine a realistic example.

Synced patterns

As shown in the PR description, the effect of synced pattern support is a populated innerBlocks property for core/block blocks.

Synced patterns do have UI in the block editor, but Block Data API users are almost certainly aware that they are not supported.

  1. I am a Block Data API consumer and I don't use synced patterns. Upgrading has no effect.
  2. I am a Block Data API consumer and I use synced patterns. I created my own implementation to resolve the synced patterns using the returned ref attribute. Upgrading has no effect since my implementation governs and overrides. I can remove my implementation at any time of my choosing.
  3. I am a Block Data API consumer and I use synced patterns. My consuming code explicitly ignores core/block blocks since they have no content. Upgrading has no effect.
  4. I am a Block Data API consumer and I use synced patterns. My consuming code effectively ignore core/block blocks because they currently have no inner blocks. But if they did have inner blocks, my code would render them. Upgrading has an effect, and I am potentially caught off guard by content that is now rendered.

Scenarios 1–3 are not breaking changes. A breaking change via scenario 4 is possible but unlikely IMO.

In summary, we have some theoretically possible breaking changes for you to consider. If we do add a v2 namespace, we require Block Data API users who want to gain support for these emerging WordPress core features to (1) notice that these features are available (2) update their code to point to the new endpoint.

In addition, we perhaps set a precedent. When this plugin adds support for new core features that alter the content—but not the shape—of the response, are we obligated to add a new version namespace? Perhaps. With enhancements to block bindings and patterns coming, this question will probably come up again.

@alecgeatches
Copy link
Contributor

@chriszarate Good points. I think we should stick with v1. In short, given that block bindings are inert with the current Block Data API and unlikely to be relied upon for functionality, this is an improvement that opens new patterns up to future API consumers without disruption.

Copy link
Contributor

@ingeniumed ingeniumed left a comment

Choose a reason for hiding this comment

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

Something minor worth pointing out in our release notes: The order of the attributes has changed in the rest api as well as in the graphQL api. I think it's minor as we shouldn't be worried about that order changing, but just worth noting.

Otherwise, love this! Thanks for working on it @chriszarate

@alecgeatches alecgeatches merged commit 74d3dda into trunk Aug 30, 2024
3 checks passed
@alecgeatches alecgeatches deleted the add/block-bindings-support branch August 30, 2024 20:55
@alecgeatches alecgeatches mentioned this pull request Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants