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] Update/build individual blocks #6087

Closed
wants to merge 60 commits into from
Closed

[WIP] Update/build individual blocks #6087

wants to merge 60 commits into from

Conversation

kanishkdudeja
Copy link

@kanishkdudeja kanishkdudeja commented Apr 9, 2018

This is continuation of work started by @aduth in #4841

Most of the work is complete. Some things which are left are:

  • Remove tuples from Webpack configuration
  • Fix some other issues in Webpack configuration
  • Fix existing unit tests
  • Fix existing end-to-end tests
  • More manual testing to make sure that nothing has broken

aduth and others added 20 commits February 2, 2018 15:30
Partially completed server-side registration for more core blocks. Some core blocks are still left.

Only editor_script handles are being enqueued as of now.
Added initial support for server-side registration for core dynamic blocks like Reusable block, Categories block and Latest posts block
Registered image block on the server-side.
…h block

WebPack was configured to bundle CSS of all blocks in one single file. Since we now need to enqueue each CSS separately, the configuration has now been updated to generate different CSS files for each block
Earlier, we were enqueuing a monolithic bundle for the CSS. Now, common structural CSS for each block is being enqueued separately.

This is needed to ensure that we can later intelligently enqueue these assets (load CSS only for blocks present on the page)
Earlier, we were enqueuing a monolithic bundle for the editor-specific CSS. Now, editor specific CSS for each block is being enqueued separately.
Since editor-specific styles for individual blocks are enqueued separately now, there is no need to register the 'wp-edit-blocks' style handle now.
…butes for individual blocks

Since the Webpack configuration now builds individual blocks to /build/blocks/library/, relative paths of style, editor_style and editor_script attributes have been changed in server side code for individual blocks.
Since the Webpack configuration now builds scripts for core-data and viewport in build/ folder, the same have now been updated in client-assets.php
We don't need "setDefaultBlockName" in heading block's JavaScriot. So removed the import.
Removed import for registerCoreBlocks in editor/test/selectors.js since it's no longer used.
Replaced name by 'core/paragraph' in paragraph/index.js since the name constant is no longer defined
@gziolo
Copy link
Member

gziolo commented Apr 10, 2018

Is it possible to automate the PHP files part to avoid so much boilerplate code? Given that all files are generated by Webpack and we know all the patterns how JS and CSS files are named based on the folder name, can we autodetect those files and register them behind the scenes?

I provide an example for Audio block.

Before

function register_core_audio_block() {
	wp_register_script( 'core-audio-block', gutenberg_url( '/build/blocks/library/audio.js' ) );
	wp_register_style(
		'core-audio-block',
		gutenberg_url( '/build/blocks/library/audio.css' ),
		array(),
		filemtime( gutenberg_dir_path() . 'build/blocks/library/audio.css' )
	);
	
	wp_style_add_data( 'core-audio-block', 'rtl', 'replace' );
	wp_register_style(
		'core-audio-block-editor',
		gutenberg_url( '/build/blocks/library/audio_editor.css' ),
		array(),
		filemtime( gutenberg_dir_path() . 'build/blocks/library/audio_editor.css' )
	);
	
	wp_style_add_data( 'core-audio-block-editor', 'rtl', 'replace' );
	register_block_type( 'core/audio', array(
		'style' => 'core-audio-block',
		'editor_style' => 'core-audio-block-editor',
		'editor_script' => 'core-audio-block',
	) );
}
add_action( 'init', 'register_core_audio_block' );

After

function register_core_audio_block() {
	register_block_type( 'core/audio', array(
		'style' => 'core-audio-block',
		'editor_style' => 'core-audio-block-editor',
		'editor_script' => 'core-audio-block',
		'detect_files' => true, // Not sure if we need it all, but leaveing a note to better explain the idea.
	) );
}
add_action( 'init', 'register_core_audio_block' );

@kanishkdudeja
Copy link
Author

Yes I agree @gziolo. This thought came to my mind as well.

@aduth What are your thoughts on this?

Kanishk Dudeja added 4 commits April 10, 2018 18:36
…block assets

The previous Webpack configuration was building individual block assets to /build/blocks/library/paragraph.js (/css)

It has now been changed to /build/__block_paragraph.js (/css)
…oks to be CSS used by the editor

Renamed style.scss in blocks/library/block to editor.scss since it looks to be CSS used by the editor.
Also enqueued the same as the editor_style attribute for 'core/block' server-side.
@aduth
Copy link
Member

aduth commented Apr 10, 2018

My only concern is that if it's boilerplate for us, it's boilerplate for any plugin developer, and we ought not give ourselves too many shortcuts if there's a solution which could benefit everyone.

And since these are meant to be proper script and style handles, when would they be registered? Could plugins still manipulate them? What is the naming convention which associates a script handle with the detected file?

@aduth
Copy link
Member

aduth commented Apr 10, 2018

Will we risk any regression on #4514 where registration was moved to registerCoreBlocks? I think I originally thought it would be no issue, since the root problem was that registration and the core library of blocks were contained within the same module, which is no longer the case here. But we should be certain.

@gziolo
Copy link
Member

gziolo commented Apr 11, 2018

My only concern is that if it's boilerplate for us, it's boilerplate for any plugin developer, and we ought not give ourselves too many shortcuts if there's a solution which could benefit everyone.

I thought about it as a general opt-in solution, rather than tailored to needs of Gutenberg (core). We should still provide an option which gives very granular control over registered assets as it is coded in this PR. I would like us to explore if there is a nice way to simplify it for recurring patterns we can observe for core blocks. Looking at Webpack config I can see that we are expecting to have index.js, styles.scss and editor.scss files.

And since these are meant to be proper script and style handles, when would they be registered? Could plugins still manipulate them? What is the naming convention which associates a script handle with the detected file?

First, I missed one thing - dependencies of the styles and scripts. They weren’t included initially which tricked me a bit but it’s being fixed (02812b4). This becomes a weak point of my proposal, but in general, it is something that should be fixed in WordPress core sooner than later. It slowly becomes a pain point as we introduce new modules.

To your point, I don’t think it would change the existing flow a lot. My expectation was to execute the logic which registers those assets at the beginning of the register_block_type method, which doesn’t differ much from what we have at the moment. Again, I’m thinking about it as the opt-in solution for those who want to follow certain patterns in how block’s folder is structured. It could work for both ES5 and Webpack controlled blocks if we would agree on common file location. In my proposal shared earlier, I provided only the name of the handle, but we could also convert it to the array to support dependencies and custom file names. We can also assume default paths for those handles based on their types. This is how it works in WP core as observed earlier.

@gziolo gziolo added [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 Framework Issues related to broader framework topics, especially as it relates to javascript [Feature] Blocks Overall functionality of blocks labels Apr 19, 2018
@kanishkdudeja
Copy link
Author

@aduth @nb @gziolo @mcsf

One blocker with shipping this functionality is that it will enqueue stylesheets and scripts for each block type separately. This will lead to multiple HTTP requests.

We can use WordPress's inbuilt load-styles.php and load-scripts.php mechanism for bundling styles and scripts together (which will lead to not needing multiple HTTP requests).

We can think of enqueueing one bundle for all block types belonging to a particular namespace. Meaning, all styles / scripts for core block types will be served in 1 bundle. For another plugin, all scripts / styles for block types defined by that plugin will be served in another bundle.

This approach however has an issue. load-styles.php and load-scripts.php use the current WordPress version as the value of the Etag header. So if a plugin/theme developer modifies some CSS / JS for their block type, the browser will not attempt to fetch the latest content since the value of the Etag header will remain the same (since the WordPress version will remain the same).

We can add some functionality in load-styles.php and load-scripts.php to support this:

Specify the value of the Etag header as a hash of the script/style handles and their corresponding versions.

So, if we get a request to load styles for paragraph and image block like this:

/load-styles.php?load=core-paragraph-block&core-image-block

We can generate the Etag like this:

etag = hash_function( 'core-paragraph-block-v1.1-core-image-block-v8.3')

and supply this value in the Etag header in the response to the browser.

This will make sure that the next time the plugin/theme developer changes the CSS / JS for a block type (meaning changes the version for the CSS / JS of a block type), the hash function will automatically generate a new value for the Etag header and the browser will therefore fetch the latest style / script bundle.

We can add this functionality in load-styles.php and load-scripts.php as an add-on to the current functionality (to preserve backward compatibility).

Maybe, this new functionality can be triggered if a specific URL parameter is included in the request to load-styles.php / load-scripts.php. Maybe, something like: /load-styles.php?etag=content_hash.

What do you guys think of this approach? Do you think we can go ahead with this?

@gziolo
Copy link
Member

gziolo commented Apr 20, 2018

@kanishkdudeja - this is exactly what we discussed on Slack. It sounds reasonable. This is exactly what I would explore first as a simple solution. Personally, I would also try to use plugin version instead of generating the hash. Unless the plugin version is unreliable.

@kanishkdudeja
Copy link
Author

kanishkdudeja commented Apr 20, 2018

Thanks for your comment @gziolo

This is exactly what we discussed on Slack.

Yes, I just wanted to keep @aduth @mcsf and @nb in loop here as well.

It sounds reasonable. This is exactly what I would explore first as a simple solution.

Thanks. I'm starting on this now :)

Personally, I would also try to use plugin version instead of generating the hash. Unless the plugin version is unreliable.

I'm not sure if we should use plugin version as a means of bursting cache for style / script bundles.

Ideally, load-styles.php or load-styles.php shouldn't be concerned about where styles / scripts are coming from (plugins/core). They should just be supplied an array of style / script handles and they should just generate the Etag based on the requested handles and their corresponding versions. This will also help let the implementation remain generic.

@aduth
Copy link
Member

aduth commented Apr 20, 2018

Ideally, load-styles.php or load-styles.php shouldn't be concerned about where styles / scripts are coming from (plugins/core). They should just be supplied an array of style / script handles and they should just generate the Etag based on the requested handles and their corresponding versions.

I agree with this. I think your idea is valid and worth exploring. I'm not sure whether there is a backwards-compatibility worry to be concerned about here either. At worst, it would "bust" existing caches for those who have them? Proposing as a ticket on the Core Trac may also invite more feedback from those who might have reason to think that assigning the current WordPress version was a necessary decision.

@kanishkdudeja
Copy link
Author

@nb @aduth @gziolo

I've opened a ticket for this on the Core WordPress Trac: https://core.trac.wordpress.org/ticket/43825

Have proposed this patch for the enhancement: https://core.trac.wordpress.org/attachment/ticket/43825/43825.patch

@kanishkdudeja
Copy link
Author

kanishkdudeja commented Apr 24, 2018

Some related work (by @youknowriad) is close to being merged in #6351.

Some of the work is common (importing dependencies from @wordpress/blocks rather than importing them internally).

Once #6351 gets merged, I'll fix the conflicts and rebase the changes in this branch from the updated master branch.

@kanishkdudeja
Copy link
Author

Pushed a commit which resolves conflicts as a result of merging changes from master (After #6351 got merged).

This has led to some regressions in tests for this branch. Will fix those as well and post an update here once it's done.

@kanishkdudeja
Copy link
Author

@gziolo @youknowriad

I had a discussion about this PR with @aduth today and we felt that it might be better to pause work on this for now since there is a major blocker with this PR going live and since this functionality doesn't seem to be a priority as of now.

This PR will enqueue separate stylesheets and scripts for each block type in the editor and front-end contexts. This will lead to multiple HTTP requests and will hurt performance. To get around this, I thought we could use WordPress's inbuilt style and script concatenation mechanism (load-styles.php and load-scripts.php) and proposed this patch (#43825) to WordPress Core to address an issue described here.

But it looks like we'll have another problem. Both these scripts don't initialize plugins. So they don't have a list of style/script handles of registered block types.

So, either we could

  • Initialize plugins in these scripts for them to be able to have style/script handles for block types (Will need to check if that could cause problems)
  • Or write a custom bundling/concatenation mechanism for this (One possible approach described here).

What are your thoughts?

@gziolo
Copy link
Member

gziolo commented May 10, 2018

Yes, this one is complex. Let's wait until we have a better mechanism for server-side registration of blocks and support for batching plugin related files in core.

@aduth
Copy link
Member

aduth commented Sep 13, 2018

This pull request appears to have languished and will not be easily reconciled with master. Please feel free to reopen and rebase against the current master, or open a new pull request.

@aduth aduth closed this Sep 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks Framework Issues related to broader framework topics, especially as it relates to javascript [Status] In Progress Tracking issues with work in progress [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.

None yet

3 participants