Build: Split packages and blocks to their webpack configs #1412

gziolo · 2021-06-22T14:25:01Z

Trac ticket: https://core.trac.wordpress.org/ticket/53690

It aligns with the changes ~~proposed~~ added in Gutenberg: WordPress/gutenberg#33293.

The idea here is to split the growing webpack config into two parts:

blocks
packages

We need to add handling for JavaScript files that are going to be used with blocks on the frontend. They don't work quite well with the current setup for entry points created for packages because of reasons like

the output path is different
frontend scripts should not be exposed under wp globals

In general, it looks like the split is a good strategy because it nicely separates two independents parts: packages and blocks. There isn't that much overlap between configs anyway.

Add View Script support for block types

As part of the effort, this PR adds support for viewScript in block.json metadata file that is later translated to $view_script in WP_Block_Type class and exposed as view_script from the REST API endpoint for block types.

It would probably be better to have the changes related to the view script in its own ticket, but it's a a bit tricky because we the split for the webpack config into two parts doesn't conceptually make too much sense if the part for blocks don't handle JavaScript files 😅 I'm happy though to split it into two commits if that makes the validation process easier for everyone.

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

gziolo · 2021-06-22T14:31:21Z

tools/webpack/blocks.js

+				),
+			} ),
+			new DependencyExtractionPlugin( {
+				injectPolyfill: true,


It looks like the option for combining assets into one file doesn't quite work with multiple webpack configs so I left it disabled for blocks for now since we have only one file for now. It will require upstream changes.

The issue is that it only outputs only one file for the config that was processed last ...

gziolo · 2021-06-22T14:38:01Z

@youknowriad and @desrosj, let me know what do you think. This is a prerequisite if we still want to wire the JavaScript file necessary for PDF embeds for the FIle block to WordPress 5.8.

gziolo · 2021-06-24T16:36:35Z

fe0d7d0 provides the way to enqueue the view script for the File block on the front end:

src/wp-includes/blocks/file.php

src/wp-includes/blocks/file/block.json

gziolo · 2021-06-25T12:41:18Z

This patch depends on #1430. Once the planned changes to WordPress npm packages are published to npm and committed to WP core, this branch can be rebased, retested and committed.

gziolo · 2021-06-25T17:08:44Z

.gitignore

@@ -29,6 +29,9 @@ wp-tests-config.php
 /src/wp-includes/css/*.min.css
 /src/wp-includes/css/*-rtl.css
 /src/wp-includes/blocks/**/*.css
+/src/wp-includes/blocks/**/*.js


The same changes need to be reflected in SVN for the file subfolder.

gziolo · 2021-06-25T17:15:13Z

I can confirm that it works as expected with all the latest changes applied to the patch.

Inline PDF embed enabled

The view script is included in the HTML source of the page together with the polyfill script:

Note: It looks like wp-i18n that depends on wp-hooks is added even when there are no translations detected in the script. I guess it's the default behavior to assume that. It should be investigated separately.

Addressed with 446a3d5

Inline PDF embed disabled

The view script is not included in the HTML source as expected:

youknowriad · 2021-06-28T13:10:48Z

This is looking good to me. @desrosj mind doing a quick review here?

desrosj · 2021-06-29T14:07:14Z

I'm seeing the following notice locally:

Notice: register_block_script_handle was called incorrectly. The asset file for the "viewScript" defined in "core/file" block definition is missing. Please see Debugging in WordPress for more information. (This message was added in version 5.5.0.) in /var/www/src/wp-includes/functions.php on line 5535

Is this because an update to the file block needs to be published? Otherwise it seems to work well for me.

desrosj · 2021-06-29T14:16:52Z

I did just notice that the Text link settings is really tall with this applied. It does not display this way before applying this PR and running the build script.

youknowriad · 2021-06-29T14:25:09Z

I did just notice that the Text link settings is really tall with this applied. It does not display this way before applying this PR and running the build script.

I'm pretty sure that's unrelated, I did get these tall selects sometimes (outside this PR) but we were not able to reproduce consistently yet. There are probably multiple factors involved there.

Notice: register_block_script_handle was called incorrectly. The asset file for the "viewScript" defined in "core/file" block definition is missing. Please see Debugging in WordPress for more information. (This message was added in version 5.5.0.) in /var/www/src/wp-includes/functions.php on line 5535

This is definitely related to this PR, I need to investigate it more, I wish @gziolo was around :)

youknowriad · 2021-06-29T14:38:34Z

I'm not able to see the notice, though it can be my docker config hiding it I don't know 🤔

youknowriad · 2021-06-29T15:05:05Z

After several tries, I'm still not able to reproduce the notice. Let's continue monitor this.

youknowriad · 2021-06-29T15:22:13Z

committed in https://core.trac.wordpress.org/changeset/51259

desrosj · 2021-06-30T02:37:49Z

Unfortunately this caused a PHP notice that I was unable to solve before it was time to package RC1, so I've had to revert it and the related commits.

gziolo · 2021-07-05T07:37:05Z

I will give it another try after WordPress 5.8 is out 👍🏻

@desrosj and @youknowriad, thank you for all the help 🙇🏻

gziolo · 2021-07-16T10:04:27Z

I reopened this branch and rebased it with the latest changes from trunk.

I can see the same notice you mentioned in the CI job that runs PHPUnit tests:

I will see how we can improve it since it's quite expected because the asset file is generated with a webpack build that doesn't run on CI before unit tests.

gziolo · 2021-07-16T10:14:29Z

Committing the asset files (and removing from .gitignore) resolves the issue, but it makes the limitation of the weback plugin hightlighted in #1412 (comment) more visible.

I still think that should be fine as the first step.

gziolo · 2021-07-19T11:23:13Z

@aristath and @youknowriad – do you think we should some automated handling for the view script? At the moment it's handled manually by the File block in render_callback. I personally think about enqueuing automatically the view script when a block is rendered and render_callback isn't provided. We can safely assume that in such a case the block author wants to have the view script to be loaded on the frontend when the block is present in the HTML.

aristath · 2021-07-27T10:28:41Z

@aristath and @youknowriad – do you think we should some automated handling for the view script? At the moment it's handled manually by the File block in render_callback. I personally think about enqueuing automatically the view script when a block is rendered and render_callback isn't provided. We can safely assume that in such a case the block author wants to have the view script to be loaded on the frontend when the block is present in the HTML.

Automatically enqueueing the script when the block is rendered would make sense... I don't see why we'd require manually adding it to render_callback 👍

…HPUnit tests

gziolo · 2021-07-27T14:34:42Z

@aristath and @youknowriad – do you think we should some automated handling for the view script? At the moment it's handled manually by the File block in render_callback. I personally think about enqueuing automatically the view script when a block is rendered and render_callback isn't provided. We can safely assume that in such a case the block author wants to have the view script to be loaded on the frontend when the block is present in the HTML.

Automatically enqueueing the script when the block is rendered would make sense... I don't see why we'd require manually adding it to render_callback 👍

Added in 1791165. If the render_callback is provided then it has to be explicitly handled to account for the more complex cases like with the core blocks: File and Navigation.

gziolo · 2021-07-28T10:44:33Z

Closed with https://core.trac.wordpress.org/changeset/51501.

gziolo self-assigned this Jun 22, 2021

gziolo commented Jun 22, 2021

View reviewed changes

gziolo requested review from youknowriad and desrosj June 22, 2021 14:36

gziolo commented Jun 24, 2021

View reviewed changes

src/wp-includes/blocks/file.php Outdated Show resolved Hide resolved

gziolo commented Jun 24, 2021

View reviewed changes

src/wp-includes/blocks/file/block.json Outdated Show resolved Hide resolved

This was referenced Jun 25, 2021

Block Library: Improve view script integration to account for WordPress Core WordPress/gutenberg#32977

Merged

Update packages with fixes for WP 5.8 beta 4 #1430

Closed

gziolo marked this pull request as ready for review June 25, 2021 12:40

gziolo commented Jun 25, 2021

View reviewed changes

desrosj approved these changes Jun 29, 2021

View reviewed changes

youknowriad closed this Jun 29, 2021

gziolo deleted the update/split-webpack-config branch June 29, 2021 18:53

desrosj mentioned this pull request Jun 30, 2021

Remove .min from the File block's viewScript. WordPress/gutenberg#33082

Closed

7 tasks

This was referenced Jul 8, 2021

Plugin: Introduce tools folder with configuration files WordPress/gutenberg#33281

Merged

Build: Split config into two parts - blocks and packages WordPress/gutenberg#33293

Merged

gziolo restored the update/split-webpack-config branch July 16, 2021 09:49

gziolo reopened this Jul 16, 2021

gziolo mentioned this pull request Jul 19, 2021

Blocks: Further improve assets handling for blocks WordPress/gutenberg#33542

Closed

11 tasks

gziolo added 11 commits July 27, 2021 16:29

Build: Split packages and blocks to their webpack configs

ad8a15f

Temporary fix to serve the view file for the File block

9536df5

Add unit tests covering new block type property

bed4ba3

Revert formatting changes to test JSON file

ae6adb3

Revert temporary changes to files copied in the build process

6f0c9df

Update the reference to the view file for File block in webpack config

de1635b

Revert changes to styles handling

4cc77b9

Include script translations only when wp-i18n is set as a dependency

5574b6c

Include generated asset files to get rid of the notice when running P…

d512505

…HPUnit tests

Include view_script in the REST API endpoint for blocks

a30b8f8

Automatically enqueue view_script when the block is rendered

1791165

aristath approved these changes Jul 28, 2021

View reviewed changes

gziolo closed this Jul 28, 2021

gziolo deleted the update/split-webpack-config branch July 28, 2021 10:44

swissspidy mentioned this pull request Aug 5, 2021

Add initial review of plugin swissspidy/gutenberg-bento#2

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build: Split packages and blocks to their webpack configs #1412

Build: Split packages and blocks to their webpack configs #1412

gziolo commented Jun 22, 2021 •

edited

Loading

gziolo Jun 22, 2021

gziolo commented Jun 22, 2021

gziolo commented Jun 24, 2021

gziolo commented Jun 25, 2021

gziolo Jun 25, 2021 •

edited

Loading

gziolo commented Jun 25, 2021 •

edited

Loading

youknowriad commented Jun 28, 2021

desrosj commented Jun 29, 2021

desrosj commented Jun 29, 2021

youknowriad commented Jun 29, 2021

youknowriad commented Jun 29, 2021

youknowriad commented Jun 29, 2021

youknowriad commented Jun 29, 2021

desrosj commented Jun 30, 2021

gziolo commented Jul 5, 2021

gziolo commented Jul 16, 2021

gziolo commented Jul 16, 2021 •

edited

Loading

gziolo commented Jul 19, 2021

aristath commented Jul 27, 2021

gziolo commented Jul 27, 2021

gziolo commented Jul 28, 2021

Build: Split packages and blocks to their webpack configs #1412

Build: Split packages and blocks to their webpack configs #1412

Conversation

gziolo commented Jun 22, 2021 • edited Loading

Add View Script support for block types

gziolo Jun 22, 2021

Choose a reason for hiding this comment

gziolo commented Jun 22, 2021

gziolo commented Jun 24, 2021

gziolo commented Jun 25, 2021

gziolo Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

gziolo commented Jun 25, 2021 • edited Loading

Inline PDF embed enabled

Inline PDF embed disabled

youknowriad commented Jun 28, 2021

desrosj commented Jun 29, 2021

desrosj commented Jun 29, 2021

youknowriad commented Jun 29, 2021

youknowriad commented Jun 29, 2021

youknowriad commented Jun 29, 2021

youknowriad commented Jun 29, 2021

desrosj commented Jun 30, 2021

gziolo commented Jul 5, 2021

gziolo commented Jul 16, 2021

gziolo commented Jul 16, 2021 • edited Loading

gziolo commented Jul 19, 2021

aristath commented Jul 27, 2021

gziolo commented Jul 27, 2021

gziolo commented Jul 28, 2021

gziolo commented Jun 22, 2021 •

edited

Loading

gziolo Jun 25, 2021 •

edited

Loading

gziolo commented Jun 25, 2021 •

edited

Loading

gziolo commented Jul 16, 2021 •

edited

Loading