Skip to content
This repository has been archived by the owner on Jul 28, 2023. It is now read-only.

Create dynamic blocks to check the DX #63

Closed
wants to merge 13 commits into from

Conversation

SantosGuillamot
Copy link
Contributor

I'm working on adding some dynamic blocks to better understand how it is the developer experience and the challenges we could face in the future.

The idea is to end up with really similar blocks to the ones we have right now but add something dynamic like the Post Title or Post Date. These are some of the steps/challenges I face prior to opening this Pull Request:

1. The view.js file wasn't being sent to the client

At first, the view.js file was overwritten and it wasn't included in the build. Thanks to @DAreRodz , we figured out that we have to enqueue the script manually in the php file. Something like this:

wp_enqueue_script('bhe-dynamic-interactive-parent-view-script');

I also struggle to find the exact string we had to use but David helped me figure it out as well.

2. Reuse dynamic data from the server to the client

In these cases, we will probably need to be able to render the dynamic parts (like the Post Title or the Post Date) in the view.js. For doing that, we could use the REST API or add this data to the block attributes. We have used the second approach so far. At first, we started using a <span> element with a class selector, but, if there are many attributes needed, @luisherranz suggested it could be cleaner to use something similar to "state":

Previously with <span>

<span class="dynamic-child-block-post-date">

Right now

<script class="dynamic-child-block-state" type="application/json">
        <?= wp_json_encode($state) ?>
</script>

3. The children and block title weren't save

The parent blocks, both the static and dynamic ones, add the possibility to edit the block title and add some children. However, at the beginning, I had some issues because in the dynamic ones this wasn't working until I figured out I had to add them in the save as well. In dynamic blocks, the save is not supposed to be needed, but it is for the editable content I believe.

4. Start using render.php

Once this Pull Request was merged, @luisherranz helped to start using the new render property. Previously we were using the traditional index.php, but this new method is much cleaner.

The only issue we found so far with the render way is that it does not bundle the render.php files in the build directory, which means we had to point to the original source. Something like file:../../../src/blocks/dynamic-interactive-child/render.php.


Bear in mind that this is still a WIP and I will keep improving the blocks and share the process here.

@gziolo
Copy link
Member

gziolo commented Sep 5, 2022

The only issue we found so far with the render way is that it does not bundle the render.php files in the build directory, which means we had to point to the original source. Something like file:../../../src/blocks/dynamic-interactive-child/render.php.

There is a way to copy PHP files to the build folder when using wp-scripts build. There is the --webpack-copy-php flag you can pass.

However, I think we can do better and detect render in block.json to copy it automatically even without any flag.

@luisherranz
Copy link
Member

I guess we should add something here. Is that correct, @gziolo? If so, any clue of what we need to add?

@gziolo
Copy link
Member

gziolo commented Sep 5, 2022

I guess we should add something here. Is that correct, @gziolo? If so, any clue of what we need to add?

It's probably a mix of changing the lines you highlighted and:

https://github.com/WordPress/gutenberg/blob/35b0bd043e7090fc851f5f62e20d3bf9c16ed02c/packages/scripts/config/webpack.config.js#L39-L41

My guess would be that we replace the lines I shared with (so probably inline the variable):

const copyWebpackPatterns = '**/{block.json,*.php}';

Instead, we could use filter option to handle the list of allowed PHP files dynamically:

  1. All files when process.env.WP_COPY_PHP_FILES_TO_DIST provided.
  2. Or only render files detected in the processed block.json files.

@luisherranz
Copy link
Member

Sounds good 👍

@SantosGuillamot
Copy link
Contributor Author

I have created this Pull Request to address it. I still want to review it but the code was working locally for me.

@gziolo
Copy link
Member

gziolo commented Sep 6, 2022

@SantosGuillamot, thank you so much for opening this Pull Request. It's going to be a very nice enhancement.

@SantosGuillamot
Copy link
Contributor Author

I'm having some issues reusing dynamic data (like Post Title or Post Date) in the server and the client using this method explained in the opening comment:

<script class="dynamic-child-block-state" type="application/json">
        <?= wp_json_encode($state) ?>
</script>

It seems that if the page is loaded quickly, the state defined in PHP and sent as an attribute is undefined in the view.js files. These are the two examples I'm talking about: Title & Date.

I've made the following video to show the issue I'm facing. If the page is loaded normally, it almost always returns the error. However, if I change the network to Fast 3G, it works fine.

https://www.loom.com/share/08bf09f4a6434c3791d5d719fb5a2eb0

Any idea why this is happening or how this should be solved?

@luisherranz
Copy link
Member

I have no idea right now, but I'll try to debug it later.

@DAreRodz
Copy link
Collaborator

DAreRodz commented Sep 14, 2022

I have been debugging the issue with Mario and it looks like the setTimeout callback we are using inside connectedCallback is running before the HTML parsing has finished. To see the problem, you can simply set some breakpoints inside those functions.

It seems the setTimeout technique is not reliable. 🤷

Maybe we could implement something that depends on DOMContentLoaded. From MDN docs:

if (document.readyState === 'loading') {  // Loading hasn't finished yet
  document.addEventListener('DOMContentLoaded', doSomething);
} else {  // `DOMContentLoaded` has already fired
  doSomething();
}

However, I don't understand why it is failing now in this branch and not before... 🤔

@luisherranz
Copy link
Member

luisherranz commented Sep 15, 2022

This is terrible. Definitely the worst part of the custom elements.

The problem with DOMContentLoaded is that it doesn't help when content appears after that event is sent. For example, when a parent component is toggling the visibility of something on and off:

const Show = ({ show, children }) => show && children; // children contains `<wp-block>`

We need something more reliable.

Jake Archibald talks about this problem in this video. He proposes:

  • A weird custom element hack that can inform the parent when it's rendered, because at that moment, all the children should be rendered as well:

    <wp-block>
      <!-- children is processed first -->
      children...
      <!-- then this final element is processed -->
      <children-finished-processing></children-finished-processing>
    </wp-block>
  • Use mutation observers.

@SantosGuillamot would you mind opening a new issue for this and mentioning it in the Custom Element's Tracking Issue so we can discuss the possibilities?

@DAreRodz
Copy link
Collaborator

DAreRodz commented Sep 15, 2022

The problem with DOMContentLoaded is that it doesn't help when content appears after that event is sent.

You are totally right; I haven't thought of that problem. ☹️

  • Use mutation observers.

This could work for subsequent <wp-block> rendering, but I think I've read that mutation observers cannot be used during the initial HTML parsing. 🤔 Not 100% sure, though.

EDIT: Or did you mean using both things at the same time? 😄

@SantosGuillamot
Copy link
Contributor Author

SantosGuillamot commented Sep 16, 2022

@SantosGuillamot would you mind opening a new issue for this and mentioning it in the Custom Element's Tracking Issue so we can discuss the possibilities?

Done 🙂 I added it to the "Required to expose as experimental API in Gutenberg" section. Feel free to move it if it shouldn't be there.

@luisherranz
Copy link
Member

EDIT: Or did you mean using both things at the same time?

I don't know but let's move the discussion to the new issue 🙂

@gziolo
Copy link
Member

gziolo commented Sep 23, 2022

a26049a - nice work ❤️


I just double-checked, and the path like those used before would get ignored by webpack because they live outside of the source directory with blocks. So technically speaking, both ways would work.

@luisherranz
Copy link
Member

Hey @SantosGuillamot, is this still relevant?

@SantosGuillamot
Copy link
Contributor Author

No, I think we can consider this experiment finished 🙂

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants