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

[WIP] Blocks: Explore how we could divide server and client block properties #5652

Closed
wants to merge 4 commits into from

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Mar 16, 2018

Description

Related to #2751.

In this PR I tried to explore one of the possible ways of having block registration moved server side. At the moment it is quite confusing that you need to register your dynamic block on both the server and client. It is more intuitive for static blocks, as everything happens on the client, but it introduces other limitations as we don't have access to the internals of WordPress. Examples:

  • Verify 'upload_files' capability when displaying upload UI in media blocks #4155 tries to disallow editing block when a user doesn't have required capabilities - it might be much easier to do the check on the server and set a proper flag there.
  • It would be much easier to set a default value for an attribute on the server when it can have a dynamic value based on some PHP dependent code.
  • Blocks: Refactor blocks to use supports align #5099 tries to add an attribute based on the supports flag, but it is not clear how to handle the case where you need to expose the same attribute on the server for the render callback. It would require making sure that the same hook is executed on both server and client.

The general idea behind this PR is to split which attributes are declared on the server and keep the rest on the client. As discussed on Slack, we might want to rename the function on the client to reflect the fact that the server registers the block, but on the client, we only add the missing parts which are JS based and UI specific like edit, save or icon (SVG involved here).

This is what I explored in this PR:

  • The fields registered on the server are moved to the JSON file.
  • Gutenberg is able to register blocks by performing auto-discovery of the aforementioned JSON files. There is no need to call register_block_type. It happens behind the scenes.
  • On the client only UI related fields are registered.
  • All the fields registered on the server can't be overridden by the client side code.
  • We would have to be renamed registerBlockType to a different name which better reflects what happens.

Issues discovered:

  • As much as I'd like to have title or description located in the JSON file it seems to be not possible as they can't be wrapped with translation functions this way.
  • It still doesn't align with the plugins and they way they register blocks. We should also look if we want to enqueue every block as a separate file or bundle them somehow. In general, it should be similar.
  • When there is a block present in the post content and you don't load all the code required to work it gets removed from the post ... What should we do in such case? Should we have a default handler which converts it to HTML block?

How Has This Been Tested?

TBD

Types of changes

Technical exploration.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.

@gziolo gziolo added [Feature] Block API API that allows to express the block paradigm. [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible [Status] In Progress Tracking issues with work in progress [Feature] Extensibility The ability to extend blocks or the editing experience labels Mar 16, 2018
@gziolo gziolo self-assigned this Mar 16, 2018
@gziolo gziolo requested a review from a team March 16, 2018 00:06
* @since 2.5.0
* @var string
*/
public $icon;
Copy link
Member Author

Choose a reason for hiding this comment

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

This probably should stay on the client as it is UI related.

Copy link
Contributor

Choose a reason for hiding this comment

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

IS this path to file or the SVG itself? If it's path to file, I think it's easier if it's done in PHP.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will have to double check, but I believe you can provide JSX there.

Copy link
Member

Choose a reason for hiding this comment

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

It's just the name of the Dashicon to use.

Copy link
Member Author

Choose a reason for hiding this comment

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

In core we use Dashicons, but it also works with SVG components, see https://github.com/WordPress/gutenberg/blob/master/blocks/block-icon/index.js.

@gziolo
Copy link
Member Author

gziolo commented Mar 18, 2018

/cc @zgordon @Shelob9 @brichards @aduth @mtias @youknowriad

@Shelob9
Copy link
Contributor

Shelob9 commented Mar 19, 2018

Replying to @gziolo -

When there is a block present in the post content and you don't load all the code required to work it gets removed from the post ... What should we do in such case? Should we have a default handler which converts it to HTML block?

What if the code missing was a custom capability/ authorization check?

As much as I'd like to have title or description located in the JSON file it seems to be not possible as they can't be wrapped with translation functions this way.

Attribute default values also may need to be translated. Not sure what to do about that, as I love this idea of registering with JSON.

@Shelob9
Copy link
Contributor

Shelob9 commented Mar 20, 2018

What about a filter for the settings so that we can optionally translate strings like placeholders and defaults?

screen shot 2018-03-20 at 12 19 57 pm

@aduth
Copy link
Member

aduth commented Mar 20, 2018

Can you explain the motivation for wanting a JSON file in the first place? Block discovery sounds nice, but I have no reason to think this would be intuitive for a developer; or, at least, no more intuitive than simply registering a block in PHP. Introducing a third file to help deal with our concerns over two files is perhaps flawed, even before considering drawbacks such as inability to localize text (or other dynamic content trivial to accomplish in PHP).

I agree with the general concern that registering a block twice is not sensible. The current behavior is such that registering on the client extends settings which are provided from the server. As noted in the original comment, perhaps we need to be more explicit that this is an extension of settings (extendBlockType), which may in-fact work well with extensibility of existing blocks if this is so desired. At the same time, we'd want to refactor our bootstrapping logic to treat server definitions as a formal registration that occurs on behalf of the block implementer (an initial registerBlockType handled automatically in the client for server-registered blocks).

@gziolo
Copy link
Member Author

gziolo commented Mar 21, 2018

Can you explain the motivation for wanting a JSON file in the first place? Block discovery sounds nice, but I have no reason to think this would be intuitive for a developer; or, at least, no more intuitive than simply registering a block in PHP. Introducing a third file to help deal with our concerns over two files is perhaps flawed, even before considering drawbacks such as inability to localize text (or other dynamic content trivial to accomplish in PHP).

I assumed that having JSON file will allow providing auto-discovery for both JS and PHP codebases in a sense that we wouldn't have to call those register methods explicitly. Another reasoning was that it is going to be possible to load this data directly without exposing them in HTML code. It seems like it is isn't possible because to satisfy a few different requirements we should expose a hook on PHP side to make it possible to perform some operations on attributes and other block properties (similar to what @Shelob9 suggested). This was also recognized as a blocker for blocks to use supports align as discussed in here: #5099 (comment). Long story short - we need to have one place where properties get updated to avoid code duplication and confusion. That's why it seems like we should explicitly pick what happens on the server and what on the client.

Having said that, I'm closing this PR with a plan to open a follow-up PR later this week, where I will propose an alternative solution that is going to do the following after the transition period:

  • register_block_type is going to be the only way to register block and it needs to happen on the server
  • attributes, supports, category, title, description will have to be registered on the server. It will be possible to update them only with the PHP hook.
  • All other properties will be provided on the client using extendBlockType with the new corresponding JS hook.
  • registerBlockType will remain with deprecation message for the next 2-3 releases to make sure developers have time to refactor their plugins.

Thanks for all your feedback. I will let you know when I have something working.

@gziolo gziolo closed this Mar 21, 2018
@gziolo gziolo deleted the try/move-block-registration-server branch March 21, 2018 16:37
@Shelob9
Copy link
Contributor

Shelob9 commented Mar 21, 2018

@gziolo This make sense. I have onequestion about your proposed path forward. If I register a block server-side how do I make it so only I can modify it with extendBlockType or is your intent that this function makes blocks more extensible?

I think it would be great if your answer was the second option. That gives us one single way to create or extend these block features. But, merging attributes or callback functions is complicated and sounds like we might need a priority system like we have with hooks.

@aduth aduth mentioned this pull request Mar 21, 2018
@gziolo
Copy link
Member Author

gziolo commented Mar 22, 2018

think it would be great if your answer was the second option. That gives us one single way to create or extend these block features. But, merging attributes or callback functions is complicated and sounds like we might need a priority system like we have with hooks.

We need to see how it goes. I think that at least attributes should be immutable on the client side to make sure that you have always the same set on both the client and server. This is necessary to make sure that editing part and server-side rendering are in sync. It also directly related to the supports property as we want to use it to allow modifications to the attributes with hooks. Again, it seems like it needs to be present on the server and be filterable only there to avoid confusion when a client-side update doesn't reflect the state on the server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Extensibility The ability to extend blocks or the editing experience [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants