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

Bundling Front-end Assets #5445

Open
mtias opened this issue Mar 6, 2018 · 48 comments
Open

Bundling Front-end Assets #5445

mtias opened this issue Mar 6, 2018 · 48 comments

Comments

@mtias
Copy link
Contributor

@mtias mtias commented Mar 6, 2018

Explore ways in which front-end styles for blocks could be assembled based on which blocks are used in a given page response.

Note: this is a deviation from how themes and resources have operated so far, since a theme usually adds a single stylesheet including all possible HTML a user might add, as well as handling for widgets, etc, that might never be present on a page.

The granularity of blocks, and our ability to tell on the server which blocks are being used, should open up opportunities for being more intelligent about enqueueing these assets.

Considerations

This would need to have proper hooks for disabling and extending, as there will be issues with async loading of content (like infinite scroll) and caching mechanisms if the bundle is dynamic.

Furthermore, it should at least consider how theme supplied styles for blocks might interop. See #5360

@mtias mtias added this to the Bonus Features milestone Mar 7, 2018
@aduth

This comment has been minimized.

Copy link
Member

@aduth aduth commented Mar 13, 2018

Related: #2756

Notably, this could have an impact on the front-end display of a block, as currently we must enqueue all scripts and styles for all blocks, regardless of whether a block of that particular type exists on the current screen. If the script and style handles were defined with the block, it is possible to limit this to only the blocks present in the current screen. This would, however, require that the post contents be parsed prior to display to detect blocks present in the post, to then check against all registered block types and associated script and styles handles. We should measure the performance impact of this parsing in relation to the benefit associated with filtering scripts.

@mcsf

This comment has been minimized.

Copy link
Contributor

@mcsf mcsf commented Mar 13, 2018

This would, however, require that the post contents be parsed prior to display

Two small things:

  • There could be alternatives to just-in-time parsing (like save-time parsing and annotation of a post's front-end needs), whatever their own trade-offs.
  • Even JIT parsing could be cached, perhaps with transients and a check for cache staleness.
@aduth

This comment has been minimized.

Copy link
Member

@aduth aduth commented Mar 13, 2018

Considering that as of #5042 we're processing all block comments anyways, it probably is not a far stretch to extend it one step further to finding the assets associated with the blocks for whom we are stripping the comments.

@mtias

This comment has been minimized.

Copy link
Contributor Author

@mtias mtias commented Mar 14, 2018

Yes, that seemed like a good hook for something like this to me.

@kanishkdudeja

This comment has been minimized.

Copy link

@kanishkdudeja kanishkdudeja commented Mar 19, 2018

Hello everybody,

I've started working on this task. I have a few questions. Please pardon me if any of my questions sound stupid. I'm just starting to get familiar with the project.

  • How will themes specify custom styles for blocks supplied by default by WordPress Core (like image, paragraph, gallery etc) ? Will they enqueue their stylesheet on the wp_enqueue_assets or the enqueue_block_assets action?

    Or is there is a possibility of Gutenberg picking up styles defined in the blocks/ subdirectory in the theme folder in the future? I read something on similar lines by @mtias in #5360

    Imagine having a blocks folder under your theme where you can add a css/scss file per block

  • Is there a possibility of not stripping block comments in the HTML (which is served to the front-end) in the future?

    If no, then like @aduth mentioned, this seems to be the most efficient way of doing this:

    Considering that as of #5042 we're processing all block comments anyways, it probably is not a far stretch to extend it one step further to finding the assets associated with the blocks for whom we are stripping the comments.

    Is my understanding correct?

  • As per the comment by @mtias below,

    This would need to have proper hooks for disabling and extending, as there will be issues with async loading of content (like infinite scroll) and caching mechanisms if the bundle is dynamic.

    I understand that we should give a hook for disabling this functionality in cases where content is loaded asynchronously (like infinite scroll)

    However, I couldn't understand what this means: "caching mechanisms if the bundle is dynamic."

    Can someone elaborate a little bit?

  • @mcsf: These are great ideas.

    There could be alternatives to just-in-time parsing (like save-time parsing and annotation of a post's front-end needs), whatever their own trade-offs.

    Even JIT parsing could be cached, perhaps with transients and a check for cache staleness.

    But I don't think we need to implement these if we decide to go with this route as @aduth mentioned.

    Considering that as of #5042 we're processing all block comments anyways, it probably is not a far stretch to extend it one step further to finding the assets associated with the blocks for whom we are stripping the comments.

    Reading block types while stripping block comments from the HTML and enqueueing their associated style handles doesn't seem to be much of an overhead. So, I don't believe caching will be of much help here.

    Am I understanding this correctly?

I'm hoping I've not overwhelmed everybody with so many questions.

@mcsf

This comment has been minimized.

Copy link
Contributor

@mcsf mcsf commented Mar 19, 2018

How will themes specify custom styles for blocks supplied by default by WordPress Core […] Or is there is a possibility of Gutenberg picking up styles defined in the blocks/ subdirectory in the theme folder in the future?

I think your project will put you in a position where you can actually inform this decision. :)

Is there a possibility of not stripping block comments in the HTML (which is served to the front-end) in the future?

These operations on blocks — comment stripping, asset loading optimizations — may be good enough arguments to keep front-end parsing. I also think there's room to improve parsing performance in the medium term. That said, I wouldn't be surprised to see third-party plugins promising performance gains by dropping front-end parsing — if this means that asset-loading optimization is lost, so be it, unless an ahead-of-time solution is devised (which I don't feel strongly about).

I think you're generally on the right track, @kanishkdudeja. I'll let others answer the remaining questions.

@kanishkdudeja

This comment has been minimized.

Copy link

@kanishkdudeja kanishkdudeja commented Mar 19, 2018

Thanks for your answers @mcsf. It makes things much more clear :)

@aduth

This comment has been minimized.

Copy link
Member

@aduth aduth commented Mar 20, 2018

To support per-block asset loading for core blocks, we will need to refactor existing core blocks to register as independent scripts. See #4841 for prior work here.

@kanishkdudeja

This comment has been minimized.

Copy link

@kanishkdudeja kanishkdudeja commented Mar 20, 2018

Hello everybody,

After going through the related issues (#2751, #5360) and having discussions with @mtias, @aduth and @mcsf, I believe this should be the scope of this task.

Scope

We should enqueue front-end styles for only those blocks which are present in the post/page content. This is possible since we can find out which blocks are present by parsing post_content field on the server.

  • This should work for both static blocks and dynamic blocks.
  • This should work for both core blocks (supplied by WordPress) and for custom block types registered by plugins/themes.
  • This should also work for nested blocks. If a block is nested within another block, styles of both the blocks should get enqeueued.

Disabling this functionality

  • Plugins/themes should be able to disable this functionality since this functionality will conflict with asynchronous loading (like infinite scroll). This would be typically done via a hook.

  • Some websites may wish to disable parsing for the front-end altogether to improve performance. This would typically be the case when they're okay with block comments being visible (not being stripped off) in the HTML.

    Therefore, we also need to provide a hook for this case.

Extending this functionality

  • It's not clear as to how plugin/themes might wish to extend this functionality. @mtias Do you have any thoughts here?

    But we should provide a hook for it. If not, atleast code should be written in such a way that adding a hook shouldn't be painful later.

Things to consider

Theme-supplied styles

As of now, themes can supply styles for core blocks by enqueueing a stylesheet on the wp_enqueue_assets or the enqueue_block_assets action. We should also aim to enqueue theme-supplied styles intelligently.

As per the discussion in #5360, a better approach might be to have the theme specify base and editor specific styles for each core block either through the API or through a file/folder naming conventions approach wherein a theme could have folders for each block inside the blocks/ subdirectory in the theme folder. Each of those folders could then contain theme-supplied styles for that block.

In my opinion, the scope here should be to come up with a theme-friendly way for themes to be able to supply styles to us. The approach should then enable us to be able to intelligently enqueue theme-supplied styles also.

Opinionated Visual Styles

As per the discussion in #5360, we may decide to give themes an option to opt in to be able to use opinionated visual styles for core blocks via the API. That could look something like: add_theme_support('wp_block_styles')

If we do go this route and if the current theme opts in for this feature, we should aim to intelligently enqueue the associated opinionated visual styles for blocks that are present on the page/post as well.

Next steps

  • Please let me know if my understanding is incorrect for any of the above points.

  • Also, is the scope fine or should we add / remove / change something?

  • I will post various approaches for this task by tomorrow / day after. Each approach will list pros/cons for each of the decisions we need to take. I will also discuss approaches for related considerations like theme-supplied styles.

  • Once we finalize an approach for this, I can then start writing some code :)

@aduth

This comment has been minimized.

Copy link
Member

@aduth aduth commented Mar 21, 2018

This looks great @kanishkdudeja . A few points of feedback:

  • The current documented approach for enqueueing styles is not to use enqueue_block_assets, but rather to assign handles as the value of style, editor_style, script, and editor_script properties in register_block_type. To your point on theme-supplied styles, this doesn't yet enable themes to add styles to an existing block, but I could imagine two other options to explore being either a filter on the block-specific enqueue, or an ability to extend an existing block (providing additional stylesheets) not too much unlike the idea for a client-side extendBlockType discussed in #5652
  • A first step may be to separate styles and scripts for core blocks from the current monolithic build (related: #4841).
@kanishkdudeja

This comment has been minimized.

Copy link

@kanishkdudeja kanishkdudeja commented Mar 21, 2018

Thanks for the feedback @aduth 👍

I will keep these points in mind while brainstorming on the possible approaches for this task.

@mcsf

This comment has been minimized.

Copy link
Contributor

@mcsf mcsf commented Mar 21, 2018

Solid report, @kanishkdudeja. The scope looks good, and I think you have all the right resources, esp. with what Andrew shared.

@kanishkdudeja

This comment has been minimized.

Copy link

@kanishkdudeja kanishkdudeja commented Mar 27, 2018

Hello everybody,

I've been thinking about this and have come up with the following approach.

Please give me feedback on the same :)

Approach

Pre-requisites

We will need to complete work on #4841 before taking this up since the following aspects of #4841 are pre-requisites for this task:

  • Separating styles and scripts for core blocks from the current monolithic build
  • Registering core blocks on the server-side along with their style, script, editor_style and editor_script attributes

Therefore, I can complete the work started by @aduth in #4841 and then proceed on working on this task :)

Getting a list of blocks present in the post

The first step is to get a list of blocks present in the post's content. Some approaches could be:

  • While stripping block comments from the post's HTML, we can keep saving each encountered block type into a data structure.

  • When a post is saved, we can parse the post's content to find the list of block types present in it before saving it to the database. This list can then be saved in the database, perhaps as a property of the post itself?

While both approaches seem reasonable, the first approach seems better to me here since we anyway need to strip block comments from the HTML when a post is requested and also because it doesn't need any auxiliary storage.

Enqueueing styles for blocks present in the post

Once we have a list of blocks present in the post's content, we can then think of only enqueueing styles for that list of blocks. Some potential approaches are:

Enqueue stylesheet for each block type separately?

This seems to be the simplest approach but can potentially lead to a lot of HTTP requests.

HTTP/2 Server Push?

Using HTTP/2 Server Push, the server can push resources to the client even if the client hasn't requested them yet.

This should have a significant improvement over the first approach.

However, this has 2 concerns:

  • Most WordPress sites must be on HTTP/1.1
  • Even if servers and clients (browsers) both support HTTP/2, NGINX and Apache just recently came out with support for Server Push over HTTP/2. Therefore, the probability of most sites being able to use Server Push would be miniscule.

Therefore, keeping WordPress's backward compatibility as an ideal, I am not sure if we should choose this approach since I believe there would be a lot of friction involved in getting users to get their server infrastructure upgraded to be able to use HTTP/2 Server Push.

Bundle required CSS

A reasonable option that we're left now with is to serve a single bundled CSS file. That would contain CSS for each of the blocks present in the post's content.

So, there can be two options here:

  • Static bundles: This means that for each possible combination of block types, we have a bundle pre-generated on the server.

    However, in this case, we will need to see if we can store so many bundles on the server.

    As an example: If there are 10 types of blocks, there can be a possible 210 bundles since a bundle can either contain styles for a block type or not.

    Therefore, we will need to store 210 bundled CSS files on the server. Assuming that a bundled CSS file on average takes 5 KB (Kilobytes) on disk, 1024 (210) such bundles will take 5 MB (Megabytes) on disk.

    I'm not sure if this is feasible. A better option could be dynamic bundles, as discussed below.

  • Dynamic bundles: These type of bundles will be generated on demand.

    Before serving the post's HTML to the browser, we can generate a CSS bundle for the block types present in the post on the back-end. Once the bundle is generated, we can then enqueue a stylesheet for that CSS bundle.

Caching mechanisms for dynamic bundles

Assuming that we go with dynamic bundles, we should think about caching mechanisms both on:

  • Server side: If the bundle was already generated previously, the server shouldn't need to generate it again.
  • Client side: If the browser has previously fetched the bundle content, it shouldn't need to fetch the bundle content again.

One way to accomplish this would be to generate an identifier for a bundle (depending upon which block types are present in the bundle). If a bundle is requested for the same set of block types, the same identifier should be generated so that we don't need to generate the bundle again. The same identifier can then be used in the bundle URL so that staticness of the identifier (and therefore of the bundle URL) can then facilitate a caching machine on the client (browser) side.

Once the bundle identifier is generated, we can then create a file on disk containing the bundle's CSS. That file can have this naming format:

style.bundle_identifier.css

Once the file has been created on disk, we can enqueue the same using wp_register_style and wp_enqueue_style functions. This will help in both of our objectives:

  • If the same page is requested again or if any other page containing the same set of block types is requested, the same bundle identifier will be generated and the server won't need to generate the bundled CSS file again since the file would already be present on disk.

  • Since the bundle URL will always remain the same for the same set of block types (since the bundle identifier would remain the same), clients (browsers) can also cache content for this bundle on their end.

Generating the bundle identifier

Once we have a list of block types present in the post's content, a way to generate a bundle identifier could be:

$bundle_identifier = hash_function( 'string denoting a set of block types' )

It's imperative that the string denoting a set of block types always contains the block types in a certain order. This would make sure that the same bundle identifier is generated for different posts/pages which have the same block types but in a different order in the HTML.

Also, it's important that we choose a hash function which minimizes the probability of collisions for these type of inputs.

For denoting different block types in the string denoting a set of block types, we can use the block name itself (like paragraph, image, gallery)

So, let's say there are a total of 4 block types: paragraph, image, gallery, twitter

Now, if a post contains a paragraph, image and a twitter block, the bundle identifier for the CSS for that post can be generated as follows:

$bundle_identifier = hash_function('image-paragraph-twitter') // We should order the blocks in the string supplied to the hash function in an alphabetical order

What if CSS for a block needs to be modified?

The above approach doesn't consider the possibility that we might need to modify the CSS for a block. If the CSS for a block is modified, the backend will still generate the same bundle_identifier since the block identifier (name of the block) will remain the same and therefore, the backend will not attempt to re-generate the bundle if the same bundle is already found on disk. Therefore, browsers will not get the latest CSS if a block's CSS gets modified.

To get around this problem, we need to also include some sort of versioning in the block identifier so that a new version (on CSS modification) also changes the bundle_identifier.

Now, the block identifier can become something like this rather than just the block name:

$block_identifier = $block_name . '-' . $css_version // Name of the block concatenated with the version of the block's CSS

Some versioning strategies can be:

  • First 6 characters of a MD5/SHA1 hash of the block's CSS content (so that changing the block's CSS automatically generates a new versioning identifier)
  • A static version number like 1.1 or 8.0.4
  • Timestamp when the block's CSS was changed

I like the first (MD5/SHA1 hash) approach here since it can be verified.

Therefore, now, let's say there are a total of 4 block types: paragraph, image, gallery, twitter

And these are the first 6 characters of the SHA1 hash of their CSS: y5b4k6, 5k8dn4, 2bd7c9, 9ytb4d

Now, if a post contains a paragraph, image and a twitter block, the bundle identifier can now be generated as follows:

$bundle_identifier = hash_function('image-5k8dn4-paragraph-y5b4k6-twitter-9ytb4d')

One thing that needs to be considered here is how will a Wordpress core developer supply the version (MD5/SHA1 hash) of the block's CSS to Gutenberg.

The version (first 6 characters of the MD5/SHA1 hash) can be supplied in the version parameter to the wp_register_style function.

wp_register_style (
    'gutenberg-paragraph-block',
    plugins_url( 'blocks/paragraph/style.css', __FILE__ ),
    array( 'wp-blocks' ),
    'y5b4k6' // The first 6 chars of the MD5/SHA1 hash
);

register_block_type( 'core/paragraph', array (
    'style' => 'gutenberg-paragraph-block',
) );

The process of computation of the MD5/SHA1 hash for the block's new CSS might take some manual work on part of the developer or some automation. If this feels like too much overhead, we can go with numbers or file modification timestamps as the versioning identifier.

Inter-operability with custom block types registered by plugins/themes

Since core block types will be registered (on the server-side) the same way as custom block types are right now, the above algorithm should remain the same for custom block types registered by plugins/themes as well.

The only thing that a plugin/theme developer will need to do is to also supply a version for the style to wp_register_style as illustrated above.

Inter-operability with theme-supplied styles

As of now, there is no documented / suggested way for themes to be able to supply styles for core blocks to Gutenberg.

One way that themes can do the same (keeping in mind that it should be compatible with the dynamic bundle approach discussed above) is:

If a theme wants to supply styles for one or more core blocks for Gutenberg, it should:

  • Include a blocks/ subdirectory in the theme folder which contains theme-supplied styles for each core block the theme wishes to supply visual styles for. Each block directory under blocks/ can contain two stylesheets: theme.css (for visual styles) and theme-editor.css (for editor-specific visual styles).

  • Call wp_register_style for each of the core blocks the theme wants to supply styles for. It should also pass the version number for each of the styles.

    After registering styles, these style handles can be passed to a server-side function extend_block_type for each core block. Example:

    wp_register_style (
      'mytheme-paragraph-block-visual-style',
      get_template_directory_uri() . 'blocks/paragraph/theme.css',
      array( 'wp-blocks' ),
      'y5b4k6' // The first 6 chars of the MD5/SHA1 hash
    );
    
    wp_register_style (
      'mytheme-paragraph-block-visual-style-for-editor',
      get_template_directory_uri() . 'blocks/paragraph/theme-editor.css',
      array( 'wp-blocks' ),
      'b57dn3' // The first 6 chars of the MD5/SHA1 hash
    );
    
    extend_block_type( 'core/paragraph', array (
        'theme_style' => 'mytheme-paragraph-block-visual-style',
        'theme_editor_style' => 'mytheme-paragraph-block-visual-style-for-editor'
    ) );

Therefore, now, let's say there are a total of 4 block types: paragraph, image, gallery, twitter

Now, if a post contains a paragraph and an image block and if the theme supplies visual styles for the paragraph block, the bundle identifier can now be generated as follows:

$bundle_identifier = hash_function('paragraph-y5b4k6-paragraphtheme-3hy6b3-image-5k8dn4')

This way, we can dynamically bundle theme-supplied styles as well. This above approach will bundle styles supplied by Wordpress core and theme-supplied visual styles in the same bundle.

Alternatively, we can opt to create separate bundles for both styles supplied by Wordpress core and theme-supplied visual styles and enqueue both of them separately.

This approach of letting themes supply styles will also enable plugins to be able to define a custom block type and a theme then being able to supply styles for it using the extend_block_type function. I sense that might be a use case for custom block types like forms etc registered by popular plugins.

Inter-operability with opinionated visual styles

As per the discussion in #5360, we may decide to give themes an option to opt in to be able to use opinionated visual styles for core blocks via the API. That could look something like: add_theme_support('wp_block_styles')

The above algorithm should work the same way. Just that, for each core block that is present in the post's content, we will now bundle two stylesheets (one for base structural styles and the other for opinionated visual styles).

Therefore, now, let's say there are a total of 4 block types: paragraph, image, gallery, twitter

Now, if a post contains a paragraph and an image block and if the theme opts in to be able to use opinionated visual styles, the bundle identifier can now be generated as follows:

$bundle_identifier = hash_function('paragraph-y5b4k6-paragraphvisual-7d53b4-image-5k8dn4-imagevisual-1ybe5d')

Disabling this functionality

Plugins/themes should be able to disable this functionality since this functionality will conflict with asynchronous loading (like infinite scroll). This would be typically done via a hook.

Plugins/themes might want to disable this bundling functionality as a whole or might only want to disable it for some block types.

Disabling it on a global level

Basically, if a plugin/theme wishes to disable this, we should enqueue a stylesheet containing styles for all available block types. Block types actually present in the post won't matter then.

The approach for this depends on how this functionality is coded.

If all of the dynamic bundle generation and enqueueing logic is put in a function generate_and_enqueue_dynamic_bundle and that function is attached to the wp_enqueue_scripts hook, we can give plugin/theme developers an option to disable it. They can remove that action like this:

remove_action( 'wp_enqueue_scripts', 'generate_and_enqueue_dynamic_bundle` );
Disabling it for some blocks

If a plugin/theme wishes to disable this functionality for only some block types, we can take this approach:

  • For block types for which this functionality is disabled, we can enqueue stylesheets for each of those block types separately.
  • For rest of the block types for which the functionality should remain enabled, the same bundling mechanism described above can be used.

I'm not really sure what the API to disable this functionality for specific block types should look like. Can someone advise?

Extending this functionality

It's not clear as to how plugin/themes might wish to extend this functionality. Use cases aren't obvious and might come in via feedback later. So, we should give an option for it or at-least architect our code in such a way so that extending this shouldn't be painful later.

We can probably provide an action (enqueue_assets_bundle_for_required_blocks) for this. Using this action, a plugin/theme can supply a PHP function to be executed after the dynamic CSS bundle is enqueued.

add_action( 'enqueue_assets_bundle_for_required_blocks', 'my_function' );

function my_function( $bundle_identifier, $bundle_file_path, $bundle_url ) {
  // Any custom logic can come here
  // This will be executed after the bundle is enqueued
}

Alternatively, a theme/plugin might want to execute a function after a dynamic CSS bundle is generated (rather than enqueued). We can provide an action (generate_assets_bundle_for_required_blocks) similarly for this:

add_action( 'generate_assets_bundle_for_required_blocks', 'my_function' );

function my_function( $bundle_identifier, $bundle_file_path, $bundle_url ) {
  // Any custom logic can come here
  // This will be executed after the bundle is generated
}

Next steps

  • Please let me know if anything of the above is unclear. I can try to explain it in more detail using some examples/illustrations.
  • Please let me know if I am misunderstanding something or if something I discussed above doesn't make sense.
  • Please give me feedback on the overall dynamic bundling mechanism approach. Can we go ahead with this? Or can this be improved? Or should we choose some other approach altogether?
  • Is my proposal for theme-supplied styles viable? Would love some feedback on it.
@aduth

This comment has been minimized.

Copy link
Member

@aduth aduth commented Mar 28, 2018

Very thorough overview @kanishkdudeja !

When a post is saved, we can parse the post's content to find the list of block types present in it before saving it to the database. This list can then be saved in the database, perhaps as a property of the post itself?

This could be okay, though it poses a challenge in ensuring that the post's content is kept in sync with the parsed block list.

While both approaches seem reasonable, the first approach seems better to me here since we anyway need to strip block comments from the HTML when a post is requested and also because it doesn't need any auxiliary storage.

It seems fine to me as well. I do think there are separate concerns between "strip comments" and "enumerate blocks present in post". Ideally we're not performing the same operation twice, but conversely I worry of a mega-function which both strips comments and aggregates the set of blocks present in a post.

Therefore, keeping WordPress's backward compatibility as an ideal, I am not sure if we should choose this approach since I believe there would be a lot of friction involved in getting users to get their server infrastructure upgraded to be able to use HTTP/2 Server Push.

Agree.

Bundle required CSS

To be honest, I think this can be a future optimization. If we can limit to just styles for relevant blocks, even if separate files, it's a good first step.

Aside: Since HTTP/2 was mentioned, there is some reason to believe the separate file requirement could be mitigated by multiplexing, though I'm not sure if consensus has been reached here.

Related:

You may also be interested in existing script concatenation:

https://stackoverflow.com/a/32589922/995445

Once we have a list of block types present in the post's content, a way to generate a bundle identifier could be:

This seems reasonable. There's some prior art in the form of oEmbed caching that exists in WordPress already:

https://github.com/WordPress/WordPress/blob/a4beb40d0b066bdf2aa35e0759accf1cc39e863a/wp-includes/class-wp-embed.php#L198-L201

Some versioning strategies can be:
One thing that needs to be considered here is how will a Wordpress core developer supply the version (MD5/SHA1 hash) of the block's CSS to Gutenberg.

Style versioning isn't a new requirement, and plugin/theme authors should have the same expectation that cache busting occurs by the $ver argument of wp_register_style. It's not clear to me why we'd need this hashing. Edit: Reading further, I'm guessing this is targeted mainly at core blocks? Generating a hash may be reasonable if we expect this to be too easily overlooked for a core developer, as long as the process for creating the hash is easy and reliable.

Include a blocks/ subdirectory in the theme folder which contains theme-supplied styles for each core block the theme wishes to supply visual styles for.

This is a pretty neat idea. One potential concern could be backwards-compatibility for existing themes which happen to have folders named blocks/ already, or at least gracefully handling these cases (more unlikely they have a conflicting blocks/core-paragraph.css for example).

Each block directory under blocks/ can contain two stylesheets: theme.css (for visual styles) and theme-editor.css (for editor-specific visual styles).

At least in Gutenberg, the convention we've settled on is style.scss and editor.scss, so maybe this could align. Not sure "theme" is providing more information that isn't already obvious by its location.

After registering styles, these style handles can be passed to a server-side function extend_block_type for each core block. Example:

How well would this work with child themes, where both a parent and a child may apply styles to a block?

Other options:

  • Extending with *_style properties merges into, but doesn't replace existing stylesheets assigned
  • Extending occurs by filtering a register_block_type hook, similar to what exists in the client, and plugin/theme can override default style and editor_style handles as concatenating to an array of handles.

I'm not really sure what the API to disable this functionality for specific block types should look like. Can someone advise?

I'm curious what the use case is for disabling this behavior per block.

@kanishkdudeja

This comment has been minimized.

Copy link

@kanishkdudeja kanishkdudeja commented Mar 29, 2018

Thanks for the detailed feedback @aduth :). My comments are inline.

This could be okay, though it poses a challenge in ensuring that the post's content is kept in sync with the parsed block list.

Yes. That's why I think we should go ahead with the other approach of getting block types present in the post's content while stripping block comments from the HTML.

It seems fine to me as well. I do think there are separate concerns between "strip comments" and "enumerate blocks present in post". Ideally we're not performing the same operation twice, but conversely I worry of a mega-function which both strips comments and aggregates the set of blocks present in a post.

I totally agree. I think we can look more into how this can be elegantly achieved once I start writing code for this.

Bundle required CSS

To be honest, I think this can be a future optimization. If we can limit to just styles for relevant blocks, even if separate files, it's a good first step.

If we enqueue separate styles for each block type, I felt that it would be a lot of HTTP requests. And @mtias mentioned "dynamic bundles" in the description of the issue above, so my approach naturally drifted towards it since that seems to be the most optimized approach.

I feel that the dynamic bundling mechanism I've proposed above shouldn't be complicated and shouldn't take too much time to implement.

What are your thoughts?

You may also be interested in existing script concatenation:

https://stackoverflow.com/a/32589922/995445

This looks interesting! There seems to be similar functionality for styles in load-styles.php inside wp-admin/.

We can think of this approach as well. But this will need me to re-evaluate how everything will work with this approach: opinionated visual styles, theme-supplied styles, caching mechanisms etc.

Should I evaluate this approach as well?

This seems reasonable. There's some prior art in the form of oEmbed caching that exists in WordPress already:
https://github.com/WordPress/WordPress/blob/a4beb40d0b066bdf2aa35e0759accf1cc39e863a/wp-includes/class-wp-embed.php#L198-L201

We can use something similar :)

Style versioning isn't a new requirement, and plugin/theme authors should have the same expectation that cache busting occurs by the $ver argument of wp_register_style. It's not clear to me why we'd need this hashing. Edit: Reading further, I'm guessing this is targeted mainly at core blocks?

This is needed for both core blocks and custom block types registered by plugins/themes.

Basically, we need style versioning as a means of cache bursting for dynamic bundles.

Let's say these two blocks are being used in a post: paragraph, image

If we don't use style versioning, then the bundle identifier (for the dynamic CSS bundle) will be generated as:

$bundle_identifier = hash_function( 'paragraph-image' );

On page load, the CSS bundle containing styles of both these block types will be generated on disk and the file will be named as style.bundle_identifier.css. If a user requests the same page again or requests another page which contains the same block types (paragraph, image), the same bundle identifier will be generated and since the same CSS bundle is already present on disk, there is no need to generate the file again.

Now, let's say if the CSS for the paragraph block is modified. Now, the issue is that again the same bundle identifier will be generated. This will be a problem since the CSS bundle present on disk (for the same bundle identifier) does not contain the new CSS code for the paragraph block.

Therefore, to fix this, the version of a block's style can be concatenated with the name of the block while passing the string into the hash function. Now the bundle identifier can be generated as:

$bundle_identifier = hash_function( 'paragraph-h5bdj5-image-7dh4b2' );

Therefore, now when the CSS for a block is modified, it's version (first 6 chars of it's MD5sum) will also change. Therefore, the string passed into the hash function will change and now a new bundle identifier will be generated.

This will now lead us to generate a new CSS bundle on disk (since there will be no file on disk for the new bundle identifier).

Therefore, this will serve as a cache bursting mechanism for dynamic bundles.

Let me know if this is still unclear. I can try to explain with the help of some illustration.

Generating a hash may be reasonable if we expect this to be too easily overlooked for a core developer, as long as the process for creating the hash is easy and reliable.

Yes, that should not be a problem.

This is a pretty neat idea. One potential concern could be backwards-compatibility for existing themes which happen to have folders named blocks/ already, or at least gracefully handling these cases (more unlikely they have a conflicting blocks/core-paragraph.css for example).

As you mention, a theme might already have a folder named blocks/ in it's directory. The blocks/ folder recommendation is just a guideline for themes to organize theme-supplied styles for core blocks in an intuitive way. Ultimately, we should be able to pick up styles on any path registered through the wp_register_style function and hooked using the extend_block_type function.

At least in Gutenberg, the convention we've settled on is style.scss and editor.scss, so maybe this could align. Not sure "theme" is providing more information that isn't already obvious by its location.

We can go by the same convention you mention (style.css and editor.scss). There wasn't a particular reason for choosing this convention (theme.scss and theme-editor.scss)

After registering styles, these style handles can be passed to a
server-side function extend_block_type for each core block.

Example:

How well would this work with child themes, where both a parent and a child may apply styles to a block?

Other options:

  • Extending with *_style properties merges into, but doesn't replace existing stylesheets assigned
  • Extending occurs by filtering a register_block_type hook, similar to what exists in the client, and plugin/theme can override default style and editor_style handles as concatenating to an array of handles.

These are nice ideas.

This one sounds better to me: "Extending with *_style properties merges into, but doesn't replace existing stylesheets assigned"

The word extend in extend_block_type gives an intuitive feeling that you can extend something. Therefore, this should go well with both the parent and the child theme being able to extend block styles.

Could there be other concerns with the extend_block_type approach?

I'm curious what the use case is for disabling this behavior per block.

I can't think of use cases here too. Maybe, we can skip this for now and take this up later if users demand it. For now, we can give plugins/themes an option to disable it on a global level.

@mcsf

This comment has been minimized.

Copy link
Contributor

@mcsf mcsf commented Apr 2, 2018

This looks great, @kanishkdudeja.

Re bundling, two things:

  • Aside from existing infrastructure like script-loader.php, bundling is also something for which the plugin space has been offering solutions for some time, meaning that a fully fledged solution is likely out of scope. Similarly, for caching, defer to core mechanisms (script versioning) and, if need be, to plugin space.
  • Andrew's references on versions 1.x and 2 of HTTP, with their takeaway that some bundling is still advised in an HTTP 2 world, suggest that it's still worth exploring bundling. Its granularity is now something to determine: I don't believe big monolithic bundles to be the solution, so is the solution a grouping of core- and theme-provided styles for each type? a grouping that considers the most common blocks? I don't know. :)
@kanishkdudeja

This comment has been minimized.

Copy link

@kanishkdudeja kanishkdudeja commented Apr 3, 2018

Thanks for your feedback @mcsf :)

Andrew's references on versions 1.x and 2 of HTTP, with their takeaway that some bundling is still advised in an HTTP 2 world, suggest that it's still worth exploring bundling. Its granularity is now something to determine: I don't believe big monolithic bundles to be the solution, so is the solution a grouping of core- and theme-provided styles for each type? a grouping that considers the most common blocks? I don't know. :)

I agree that a big monolithic bundle might not be the ideal solution here. Like you suggest, some ways in which we could group bundles are:

  • Bundles for each block type. In this case, each bundle would contain core and theme-provided styles for the corresponding block type.

  • Separate bundles for styles supplied by WordPress core and theme-supplied styles. Therefore, if a post contains 4 block types, one bundle would contain core styles for those 4 block types. The other one would contain theme-supplied styles for those 4 block types.

  • Separate bundles for common block types and other block types. For instance, by common, we could mean block types like paragraph, image, heading which have a high probability of being present in web pages.

So, the question we need to answer is this:

  • Should the first version implement no bundling mechanism at all? Meaning that, should it simply enqueue separate core and theme-supplied stylesheets for each block type present in the post's content? This will lead to multiple HTTP requests (for each block_type present in the content) but as @aduth mentioned, this might not be as big of a problem with HTTP/2.

  • Or should the first version implement some bundling mechanism? The granularity of bundling (as discussed above) could still be decided upon.

@mtias @nb I would love to hear your thoughts on this so that we can reach a consensus here.

@mcsf

This comment has been minimized.

Copy link
Contributor

@mcsf mcsf commented Apr 3, 2018

If, for the sake of incremental development and separation of concerns, it makes more sense to first split all assets and serve them individually, that seems fine to me, but any v1 of this needs some bundling capability, IMO.

Re bundling method, I don't really have an opinion. It would be pretty nice to be able to develop some heuristics and determine which method is best (in general, or for a given site), but I don't mean to divert from the scope of this task ;-), so I'll defer to @nb.

@kanishkdudeja

This comment has been minimized.

Copy link

@kanishkdudeja kanishkdudeja commented Apr 3, 2018

If, for the sake of incremental development and separation of concerns, it makes more sense to first split all assets and serve them individually, that seems fine to me, but any v1 of this needs some bundling capability, IMO.

I also agree here. Since I believe HTTP/2 support will still take quite a bit of time to propagate over the web. Or maybe I'm biased since the approach I proposed above uses a bundling mechanism.

Re bundling method, I don't really have an opinion. It would be pretty nice to be able to develop some heuristics and determine which method is best (in general, or for a given site)

I agree here as well :)

@aristath

This comment has been minimized.

Copy link

@aristath aristath commented Apr 3, 2018

The way I see it, most blocks won't have lots of styling anyway. I mean most will be less than 1-2kb. In some cases we may see some blocks with more than that, but still their size should be taken into account. Serving 2 files 5kb each would be more costly that a single 10kb file. If the files however are larger, then it would make sense to separate them. Perhaps we could implement a threshold? Count the total size of the styles, and if they are larger than say 100kb then split, otherwise serve as a single asset. It shouldn't be too hard to implement...

@nb

This comment has been minimized.

Copy link
Member

@nb nb commented Apr 3, 2018

@aduth @kanishkdudeja @mcsf do you have any idea what impact on real users will have either approach? What % of users have access to working HTTP/2 push infrastructure? In a fairly typical case, how much of a hit will load time take if we’re issuing a request for each stylesheet?

@aaronjorbin

This comment has been minimized.

Copy link
Member

@aaronjorbin aaronjorbin commented Apr 3, 2018

@kanishkdudeja

This comment has been minimized.

Copy link

@kanishkdudeja kanishkdudeja commented Apr 3, 2018

Thanks for your comment @nb :)

In a fairly typical case, how much of a hit will load time take if we’re issuing a request for each stylesheet?

I'll get back on this.

@kanishkdudeja

This comment has been minimized.

Copy link

@kanishkdudeja kanishkdudeja commented Apr 3, 2018

Thanks a lot for providing these numbers @aaronjorbin :)

Even without Server Push (over HTTP/2), multiplexing support in HTTP/2 should help optimize the page load experience even if we enqueue multiple stylesheets (one for each block type).

But as you mention, I'm not sure if we should rely on HTTP/2 support since ~25% of servers support it as of now and also because it's rate of growth doesn't seem to be promising.

@aduth

This comment has been minimized.

Copy link
Member

@aduth aduth commented Apr 3, 2018

Sorry, I didn't mean to derail the conversation by bringing up HTTP/2 😄 I do think that a decision on it needn't be a blocker for moving forward with some of the more immediate tasks. It seemed to me an optimization which could be bolted on after we've at least implemented the basics of understanding which blocks exist and the styles of each need to be enqueued.

I did mean to raise it in the sense of questioning assumptions based on the state of the web as it exists today, in mind of the fact that it may not be worth dedicating the bulk of the effort here toward optimizing for deprecated standards (Is it fair to call it deprecated? I honestly don't know).

@kanishkdudeja

This comment has been minimized.

Copy link

@kanishkdudeja kanishkdudeja commented Apr 5, 2018

In a fairly typical case, how much of a hit will load time take if we’re issuing a request for each stylesheet?

As per @nb's suggestion, I've been conducting some benchmarks on the performance comparison between both approaches and have some results to show.

Test Environment

  • 2 static pages are hosted using NGINX 1.13 (latest stable version as of today) on a new Digitalocean Instance. The droplet isn't running anything else to make sure that CPU contention doesn't skew benchmark metrics.
  • Tests were done using https://webpagetest.org which according to my research, seems like the best solution for conducting such tests. Google also recommends them.
  • Tests were done on HTTP/1.1 (by disabling HTTP/2)
  • DNS lookups were avoided to avoid skew in benchmark metrics. This was done using custom scripting on https://webpagetest.org.
  • Browser caching is disabled by default in https://webpagetest.org so that it also doesn't skew test results.

Test variations

These 2 web pages were tested.

This is what they have in common:

  • JQuery (minified)
  • Boostrap's CSS and JS (minified)
  • FontAwesome's CSS (minified)
  • Custom Javascript (Minified)

This is how they are different:

  • The first one (css-bundle.html) contains 1 bundle for all the custom CSS the page needs.
  • The second one (css-separate.html) contains 8 different stylesheets for these components (base, anchor, blockquote, button, heading, image, paragraph, custom)

Evaluation of test results

In my opinion, the two most important factors (taking user experience into account) for evaluating web page performance are first render time / first paint time and first interactive time

  • First render time / First paint time: This denotes the time at which the browser starts rendering something on the screen.
  • First interactive time: This denotes the time at which the user sees enough content / menu items that he/she can then start to interact with the web page.

Therefore, for both of these metrics, the smaller they are, the better.

We can also look at Total page load time as an evaluation criteria for these tests.

Test Results

Test 1

This test was performed on a desktop version of Chrome from San Fransisco.

Test 2

This test was performed on a mobile version of Chrome on Moto G4 from Dullus, Virginia.

Results

For the desktop version, the separate CSS version took ~33 extra milliseconds for the first render time.
For the mobile version, the separate CSS version took ~300 extra milliseconds for the first render time.

For each of these tests, a Waterfall view of the requests and a Connection view is shown in the Details tab.

Most browsers can open upto 6 parallel HTTP connections with a specific origin for subsequent HTTP requests for assets (after parsing the HTML).

According to my interpretation of the waterfall view for the separate CSS version for both desktop and mobile tests, it looks like the browser is waiting for HTTP connections to become idle (since all 6 are being used) to send subsequent requests for left over stylesheets. Since CSS is render-blocking by default, the browser can't begin to render anything until it has fetched all the CSS assets and has constructed the CSS DOM for them.

Interpretation of the results

The results concur with our theory that enqueueing multiple stylesheets will lead to some delays for first render, first interactive and page load times.

A delay of 50-200 ms might not seem much here. But taking an average of a page containing 8 block types, this will become a bigger problem if we enqueue theme-supplied stylesheets separately. Then the delay can go upto 100-400 ms since we will need to enqueue 16 different stylesheets (8 for core styles and 8 for theme-supplied styles,

Therefore, I think we should do some amount of bundling (something in between 1 monolithic bundle and separate stylesheets for both core and theme-supplied styles for each block type) so that we don't have the browser waiting for too many HTTP requests needing to be fired and at the same time we can use parallelism provided as a result of 6 parallel HTTP connections.

All of this will obviously vary depending upon how many assets (apart from the CSS for blocks) the web page requests for itself (like images, other stylesheets, javascript). But this example of a typical web page should give us some idea about the tradeoffs involved.

Improving the accuracy of these tests

Even though these test results are a median over 3 consecutive attempts, variable factors like network latency / congestion can skew these test results. If needed, I can try to simulate these tests on my local web server so that we can remove that factor as well.

Feedback?

I would love to hear feedback on the following:

  • Is my interpretation of test results correct? Or am I mis-interpreting something?
  • Are there other factors which could skew these test results?
  • Do you agree with the approach - enqueue multiple smaller bundles for block types present in the post's content? The granularity of grouping bundles can still be decided upon.
  • Any other feedback on these tests?
@kanishkdudeja

This comment has been minimized.

Copy link

@kanishkdudeja kanishkdudeja commented Apr 17, 2018

@kanishkdudeja If we can be assured that a given block's stylesheet are located on the filesystem (as with load-styles.php), what if the CSS for a given block were inlined in a <style> element before the first instance of each block in the page? This would avoid having to try to look ahead to figure out all of the styles that would need to be enqueued and printed in the head, and when in the body there wouldn't be the concern of FOUC or the slowdown of additional HTTP requests for blocks.

@westonruter This seems like a good idea. As you suggest, inlining CSS in a <style> element before the first instance of each block in the page will definitely ease out the concerns described in this comment.

However, it would also lead to the CSS becoming non-cacheable. Since every post would now contain inline styles for it's required block types, we will no longer have the advantage of the dynamic CSS bundle being cached. I think we should measure what impact this has on subsequent page load (after the first page load) times for a typical web page.

What do you think?

@westonruter

This comment has been minimized.

Copy link
Member

@westonruter westonruter commented Apr 17, 2018

If there were a cacheable external CSS bundles, an if every page has a different set of block types on it, then wouldn't that mean there would be a separate external stylesheet needing to be included for potentially many pages on a site? That would be bad for performance since external stylesheets block page loading, in addition to any stylesheets already enqueued for the theme itself. So for each subsequent page load there could potentially be a different bundle that needs to be loaded.

On the other hand, since styles for the blocks are required to display them properly should they not be considered critical CSS and thus important to be served with the HTML? In any case, is not the amount of CSS associated with a given block quite small anyway? To me it seems the downsides of having multiple requests to cacheable external stylesheets is greater than having a bit extra page weight to having the relevant blocks' styles inlined. The same argument is often made for/against using data: URLs for images. When the weight of the HTTP request is greater than the weight of the added page weight, the thing should be inlined. Interestingly, if you try doing a Google search you'll notice that most of the images you see have data: URLs. Same rationale I think is used for why Dashicons stylesheet has the WOFF file in a data: url as opposed to loading an external file.

@mcsf

This comment has been minimized.

Copy link
Contributor

@mcsf mcsf commented Apr 18, 2018

Not meaning to derail @kanishkdudeja's scope, I think there's merit to @westonruter's per-block style inlining idea, but it'd be useful to run some simulations on actual payloads and pay-off.

On the other hand, since styles for the blocks are required to display them properly should they not be considered critical CSS and thus important to be served with the HTML? In any case, is not the amount of CSS associated with a given block quite small anyway?

I'd like to think that blocks will always be very self-contained and described by stylesheets as compact as possible. However, the variance will likely be significant. What does this mean for optimization? I'm not sure. A hybrid approach could prove beneficial:

Pretend that determining the size of assets on the server has minimal cost. We could then envision a mechanism which inlines block styles if their total weight is below a certain threshold, or skip inlining for individual blocks if their styles are significantly heavier than the rest, thus only blocking rendering of the more "demanding" blocks.

@kanishkdudeja

This comment has been minimized.

Copy link

@kanishkdudeja kanishkdudeja commented Apr 18, 2018

@westonruter In my previous comment, by subsequent page load, I meant the same page being loaded again and again. I didn't mean navigating to a different page (which would mean requesting a different dynamic bundle as you suggest). I'm sorry for derailing the conversation since I imagine that loading the same page again and again wouldn't be a common scenario.

So for each subsequent page load there could potentially be a different bundle that needs to be loaded.

Yes, correct. But, there has been some talk of having a common bundle which will get enqueued on every page regardless (which would contain styles for commonly used block types like paragraph, image, heading block types (which we would expect to be present on most pages)). The rest of the styles can be bundled into a separate file depending upon what other block types are present in the post.

In this case, having this bundle for commonly used block types cached in the browser should make a significant performance difference.

That would be bad for performance since external stylesheets block page loading, in addition to any stylesheets already enqueued for the theme itself.

External CSS does block rendering of the content but the browser can request assets in parallel. So, if the external CSS grows to a decent size, it might be a better idea to have it fetched in parallel.

In any case, is not the amount of CSS associated with a given block quite small anyway?

Right now, we only enqueue base structural styles for core blocks supplied by WordPress Core. But we may also need to enqueue opinionated visual styles for those core blocks (if a theme wishes to enable support for them) and theme-supplied styles for core blocks (if a theme wishes to extend styles for core blocks). Both of these have been discussed in #5360 and in this thread. So it's possible that the size of the payload might grow to a decent size.

Not meaning to derail @kanishkdudeja's scope, I think there's merit to @westonruter's per-block style inlining idea, but it'd be useful to run some simulations on actual payloads and pay-off.

@mcsf I agree that doing some tests and simulations would give us further insight here.

Pretend that determining the size of assets on the server has minimal cost. We could then envision a mechanism which inlines block styles if their total weight is below a certain threshold, or skip inlining for individual blocks if their styles are significantly heavier than the rest, thus only blocking rendering of the more "demanding" blocks.

This seems a really interesting idea @mcsf :). I think this kind of a hybrid approach will perform well for both scenarios.

@ktmn

This comment has been minimized.

Copy link

@ktmn ktmn commented Jul 11, 2018

Is this issue still looked at? With potential August release I'd definitely like to sort out how my block styles are going to be working but it seems to be stagnant for 3 months.

Perhaps it's not paramount to figure out if or how to bundle the styles, but at least get it to only load the ones that are actually being used and plugins like Automatize can take care of the bundling to reduce request count.

More importantly, I'd really like to also see a more flexible way to specify block assets, rather than just script, editor_script, style, and editor_style properties which don't even take arrays.

#5740 - assets_callback seemed perfect for me (I'm bringing it up here, since it was closed and *this* issue was referenced as the alternative).

@mtias mtias added the Needs Dev label Jul 17, 2018
@aristath

This comment has been minimized.

Copy link

@aristath aristath commented Jul 21, 2018

I'm sorry for jumping in here like that, but why don't we consider using a service-worker to more efficiently cache these?

@westonruter

This comment has been minimized.

Copy link
Member

@westonruter westonruter commented Jul 24, 2018

I don't think service workers are really a fix for this. Using a service worker to cache the stylesheets does nothing to help a first time visitor to a site, when it is the most important to make a good first impression to encourage a visitor to come back. But the service worker only will get installed after first visiting the page. Nevertheless, even if a service worker is able to serve a large stylesheet from cache there is still the problem of many unnecessary CSS rules potentially being loaded into the document without them being used.

@towfiqi

This comment has been minimized.

Copy link

@towfiqi towfiqi commented May 31, 2019

@aristath

This comment has been minimized.

Copy link

@aristath aristath commented May 31, 2019

I'm doing something similar on my theme too... https://aristath.github.io/wordpress/2019/05/11/howto-print-used-blocks-styles.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Overviews
Infrastructure
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.