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

Blocks: Propose a server side rendering implementation #586

Merged
merged 17 commits into from
May 10, 2017

Conversation

youknowriad
Copy link
Contributor

This is also called dynamic blocks and is useful for blocks like embed, comments blocks, ...
This is still in progress but feedback is appreciated.

Testing instructions

  • Insert this <!-- wp:core/html -->anything<!-- /wp:core/html --> to a post content (old editor) and preview this post, you should see your block replaced by THIS IS AWESOME

@youknowriad youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label May 1, 2017
@youknowriad youknowriad self-assigned this May 1, 2017
@youknowriad youknowriad requested review from mtias and aduth May 1, 2017 14:01
@mtias mtias added the [Feature] Block API API that allows to express the block paradigm. label May 1, 2017
index.php Outdated
*/
function register_block( $name, $render ) {
global $registered_blocks;
$registered_blocks[ $name ] = $render;
Copy link
Member

Choose a reason for hiding this comment

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

I don't want to preemptively assume requirements, but do you think we'd need a block to register options aside from just a render?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to think about this, but I didn't find any use-case for now. Though, we don't know the future maybe options is preferable.

index.php Outdated

* @return string Updated post content
*/
function do_dynamic_blocks( $content ) {
Copy link
Member

Choose a reason for hiding this comment

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

This function could do for some comments; various indices are hard to follow.

Copy link
Member

Choose a reason for hiding this comment

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

Is the specificity of "dynamic" something we want to expose in the function name? I'd expect we'll add hooks here for interacting with individual blocks (say I want to filter all quote blocks for some reason). Performance here will be very important as well.

index.php Outdated
$new_content = $content;
foreach ( $matches[ 0 ] as $index => $block_match ) {
$block_name = $matches[ 1 ][ $index ][ 0 ];
if ( isset( $registered_blocks[ $block_name ] ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe worth an early continue to avoid indentation:

if ( ! isset( $registered_blocks[ $block_name ] ) ) {
	continue;
}

index.php Outdated
foreach ( $matches[ 0 ] as $index => $block_match ) {
$block_name = $matches[ 1 ][ $index ][ 0 ];
if ( isset( $registered_blocks[ $block_name ] ) ) {
$block_length = mb_strlen( $block_match[ 0 ] );
Copy link
Member

Choose a reason for hiding this comment

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

I don't know the answer, but is mb_strlen the safe default over strlen for multi-byte content?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my PHP souvenirs, it is, but I'm not in my comfort zone anymore in PHP. Other 👀 here could be useful.

index.php Outdated
@@ -247,3 +316,7 @@ function the_gutenberg_project() {
</div>
<?php
}

register_block( 'core/html', function( $attributes ) {
Copy link
Member

Choose a reason for hiding this comment

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

Seems temporary, but just for clarity, the anonymous function callable is only available in PHP versions 5.3.0 and newer (reference). WordPress supports as far back as 5.2.4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right 👍

index.php Outdated
*/
function do_dynamic_blocks( $content ) {
global $registered_blocks;
$open_matcher = '/<!--\s*wp:([a-z](?:[a-z0-9\/]+)*)\s+((?:(?!-->).)*)-->.*<!--\s*\/wp:\g1\s+-->/';
Copy link
Member

Choose a reason for hiding this comment

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

Would we want the registered block to be able to make use of the content between the comment delimiters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know there were some discussions about this, but IMO for consistency, Dynamic blocks should not contain any data between the delimiters. Their save function should return null.

Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't prescribe what save returns, because it may make sense to return a link, or a short message. Consider a block that renders the last 5 recent posts from a certain category. It may be good that when the dynamic markup can't be generated, it returns a link to /blog/category/:term with some text saying "View latest posts on :term".

@mtias
Copy link
Member

mtias commented May 1, 2017

Thanks for starting this.

@youknowriad
Copy link
Contributor Author

I'm thinking to leverage this for the embed block. But to do so, we need to change the way we store attributes (use HTML like syntax).

What do you think if

  • we remove the dummy "register_block" call.
  • Try to merge this proposal
  • do a follow-up PR to change the attributes' syntax
  • and then another PR to add the server side embed block.

(I like splitting/Iterating :) )

@aduth
Copy link
Member

aduth commented May 2, 2017

I'm not totally clear what you mean by changing attribute syntax, but I'd be fine with merging this and iterating on it in subsequent pull requests since it has no impact on existing behavior for the moment.

@nylen
Copy link
Member

nylen commented May 2, 2017

This logic looks fairly complicated and needs unit tests as soon as possible. See https://pippinsplugins.com/unit-tests-wordpress-plugins-setting-up-testing-suite/ for example.

I think we should do more to unify our block registration APIs between the client and server (require the same rules for block slugs, for example). I also wonder if we need or want some way to guarantee that if a block is registered on the server, then it's also registered on the client (and the relevant details match up).

I'm not sure those need to be done in this PR, but I'm also not sure why we need to merge this immediately. Are we ready to use it yet?

@BE-Webdesign
Copy link
Contributor

BE-Webdesign commented May 3, 2017

What does the server side API need to do, why does it need to do it, and how will it interact with the client side API, would be good questions to answer before we get too attached to this. I am fine with merging, but I think the current client API could use improvements right now as well, and we should circle back to refining the API at some point.

@westonruter
Copy link
Member

Additionally, I think there is also the question as to whether the server-side API should also be able to facilitate rendering of blocks for displaying inside of the visual editor, similar to what the Shortcake plugin does. A good case for server-side rendering for the block in the editor is both for legacy content (e.g. shortcodes) but also for displaying dynamic blocks that are not feasible to provide pure JS renderings without duplicating a lot of logic.

@youknowriad
Copy link
Contributor Author

@aduth

I'm not totally clear what you mean by changing attribute syntax

moving from attr:value to attr="value" or { attr: "value" }. We can't store URLs for example for now.

@nylen

This logic looks fairly complicated

I don't think so

needs unit tests as soon as possible

I agree

I think we should do more to unify our block registration APIs between the client and server

Sounds good for the validation

. I also wonder if we need or want some way to guarantee that if a block is registered on the server, then it's also registered on the client

I don't think so, we only need the dynamic blocks on the server, why forcing plugin authors to add something optional

but I'm also not sure why we need to merge this immediately

I'm not so impatient to merge this, I was waiting for review and you raise some good points here. But I like to split the work to smaller PRs.

Are we ready to use it yet?

Yes we are, we can use this to generate the HTML for the embed block for example.

@BE-Webdesign

What does the server side API need to do, why does it need to do it

The only thing that it needs to do for now is rendering dynamic blocks and we currently have a dynamic block on the client side (The embed block). I'm trying to start the server side implementation to tackle this particular task. We'll iterate and see if we need any other server side API.

@westonruter

Additionally, I think there is also the question as to whether the server-side API should also be able to facilitate rendering of blocks for displaying inside of the visual editor

I'm not sure I understand! Are you saying we should generate a form in the visual editor server-side? I would personally strongly argue against that and instead, build a specific "nice" UI for each block. But I understand this could be helpful for mobile apps maybe. Anyway, this is a separate task and for now, I'm trying to address the server-side rendering of dynamic blocks.

@nylen
Copy link
Member

nylen commented May 3, 2017

I'm not so impatient to merge this, I was waiting for review and you raise some good points here. But I like to split the work to smaller PRs.

I agree, I guess it makes me a bit nervous that we are building a copy of an API that already exists on the client side. We need to be careful that it works the same way as much as possible. To me that means we should follow a similar process here to #289, including the same test cases. So I'd prefer that we finish and merge #617 (PHPUnit) first, then add test cases here.

Edit: I see that you already did a lot of these updates, nice 😀

@youknowriad
Copy link
Contributor Author

Server side tests in place, any other concern here?

@nylen
Copy link
Member

nylen commented May 5, 2017

This was also present on #617, but I'm seeing a lot of warnings in the build log:

PHP Warning: filemtime(): stat failed for /home/travis/build/WordPress/gutenberg/i18n/build/index.js in /home/travis/build/WordPress/gutenberg/index.php on line 161

Example: https://travis-ci.org/WordPress/gutenberg/jobs/228752501

Also, from #617 (comment), was this ever addressed?

Take a close look at each of the jobs, there are two showing "green" that they passed, this is false, there's lots of errors on those jobs.

Let's fix the warnings here and make sure that when the build fails it actually fails like it should. I'll do a separate PR against this branch to test out the second thing.

@youknowriad
Copy link
Contributor Author

Oh! I haven't seen those errors. I thought he was talking about the failing jobs. At that moment three of five jobs were failing.

/**
* Successfull dynamic block rendering
*/
function test_register_block() {
Copy link
Member

Choose a reason for hiding this comment

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

In this PR there have been issues with matching the content inside the block delimiters. Let's get another test similar to this one but testing out content inside blocks as well:

before
<!-- wp:core/dummy value:a1 -->this should be ignored<!-- /wp:core/dummy -->
between
<!-- wp:core/dummy value:a2 -->this should be ignored also<!-- /wp:core/dummy -->
after

This is also called dynamic blocks and is usefull for blocks like embed, comments blocks, ...
 - Also, ensure PHP 5.2 compatibility
 - Adding some comments to explain the server side rendering
* Setup server side unit tests
index.php Outdated
return;
}

if ( isset( $registered_blocks[ $slug ] ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not trigger a _doing_it_wrong here too, like we do on the client side?

index.php Outdated
*/
function unregister_block( $slug ) {
global $registered_blocks;
if ( ! isset( $registered_blocks[ $slug ] ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not trigger a _doing_it_wrong here if this is set, like we do on the client side?

@nylen
Copy link
Member

nylen commented May 5, 2017

Here are the block registration test cases on the client side:

  blocks
    registerBlock()
      ✓ should reject numbers
      ✓ should reject blocks without a namespace
      ✓ should reject blocks with invalid characters
      ✓ should accept valid block names
      ✓ should prohibit registering the same block twice
      ✓ should store a copy of block settings
    unregisterBlock()
      ✓ should fail if a block is not registered
      ✓ should unregister existing blocks

Let's get all of the same tests running here too.

nylen and others added 3 commits May 5, 2017 16:59
- Do the client-side build before running PHPUnit tests; this ensures
  that the plugin doesn't try to enqueue non-existent JS files and cause
  a bunch of warnings
- If a step in the build fails, go ahead and exit
- Don't scan node_modules/ and vendor/ for PHP files
- Run phpcs with the `-s` option (show rule identifiers in reports)
- Don't require docblocks for unit tests
@youknowriad
Copy link
Contributor Author

I've added more unit tests fc7fabe (closer to what we have on the frontend). Can I have another look @nylen?

@nylen
Copy link
Member

nylen commented May 9, 2017

There were still a couple of tests missing:

Other changes:

  • As of 47e81bd every test method no longer needs to have an annotation.
  • Changed $registered_blocks to $wp_registered_blocks; in WP, prefix everything.
  • Explicit return false; for error conditions; also test accordingly.
  • You can now run phpunit from within a WP dev install without additional configuration or having to specify WP_TESTS_DIR.

Copy link
Member

@nylen nylen left a comment

Choose a reason for hiding this comment

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

If latest changes look OK: :shipit:

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. Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants