Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Blocks: Further improve assets handling for blocks #33542

Closed
9 of 11 tasks
gziolo opened this issue Jul 19, 2021 · 24 comments
Closed
9 of 11 tasks

Blocks: Further improve assets handling for blocks #33542

gziolo opened this issue Jul 19, 2021 · 24 comments
Assignees
Labels
[Feature] Block API API that allows to express the block paradigm. [Package] Block editor /packages/block-editor [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues.

Comments

@gziolo
Copy link
Member

gziolo commented Jul 19, 2021

Description

There is still some room for improvements after WordPress 5.8 release that brings some new exciting features to Block API listed in dev notes:

Based on the feedback received during the implementation and after the dev notes were published we created some follow-up tasks.

Planned tasks

@gziolo gziolo added [Feature] Block API API that allows to express the block paradigm. [Package] Block editor /packages/block-editor [Status] In Progress Tracking issues with work in progress labels Jul 19, 2021
@gziolo gziolo added the [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues. label Jul 19, 2021
@gziolo gziolo changed the title Block Editor: Further improve assets handling for blocks Blocks: Further improve assets handling for blocks Jul 19, 2021
@Mamaduka
Copy link
Member

Hi, @gziolo

Is this the correct PR for the viewScript - #32814?

@gziolo
Copy link
Member Author

gziolo commented Jul 20, 2021

Is this the correct PR for the viewScript - #32814?

There was another PR - #32977. It's all based on the work started by @vcanales in #30341 :)

@gziolo
Copy link
Member Author

gziolo commented Oct 7, 2021

Support for multiple style assets per type landed in Gutenberg: #32510

@rmorse
Copy link
Contributor

rmorse commented Jan 21, 2022

Just wanted to weigh in on this and add my support for allowing multiple scripts for both view and block.

There are two use cases that spring to mind:

1. Using regular JS libraries + packages (that are not package manager compatible)

This is my current situation - essentially, I have frontend scripts + css, and they expose an api on the window object, allowing you to generate various types of content to place anywhere on a page (using the api).

In admin / my block, rather than rewriting these scripts, I load both the CSS + JS "as is" (with some warnings, but it works) and interact with the api defined on the global window object. I then generate the various content from the api, and place them into the preview area of my block.

I think, while this is quite a specific use case, it also highlights a much broader requirement - as this could apply to any libarary / package which can't be loaded as a dependency via a package manager, or if there are scripts that we might want to share and re-use.

2. Managing admin assets more efficiently

This is not my current use case, but can see how it would be handy. Lets say I wanted to build a component library, that we'll register and share across multiple plugins and admin screens - then it would be handy to require the shared library as and when needed - and only load extra scripts if our block is going to be loaded - we could then expose that as a global that can be required by each plugin (much like gutenberg does with wp.element etc)

I'm sure its hardly a common use case, but I think it would be great to have the flexibility to do this kind of thing

@fabiankaegy
Copy link
Member

I'm on a similar page as @rmorse here. The most common reason for having to resort to php to register the script or style handle is because I need to add additional scripts to the dependency array.

@gziolo
Copy link
Member Author

gziolo commented Jan 21, 2022

@rmorse and @fabiankaegy, thank you for the feedback and for reassuring us that this functionality is going to be helpful. @aristath, do you plan to iterate on #36176 during WordPress 6.0 release cycle?

@aristath
Copy link
Member

@aristath, do you plan to iterate on #36176 during WordPress 6.0 release cycle?

Definitely!
As soon as WP5.9 is released we'll start working on performance improvements like this one for v6.0 👍

@ocean90
Copy link
Member

ocean90 commented Feb 11, 2022

I noticed that the viewScript asset is also enqueued in the editor. Did something change or is this a bug? Based on the description "It will be enqueued only when viewing the content on the front of the site." I'm assuming the later.

Core enqueues the files here. This check only does not enqueue the script if the block has a render callback. There are only two core blocks, file and navigation, which have a view script but also a render callback. So core doesn't seem to be affected directly.

I think that this might be caused by the REST API preloading which is also rendering blocks and thus the render_callback is called which again enqueues all the assets. I have to do some more digging, but maybe this is already a known issue?

Edit: I checked the debug backtrace and it's indeed the preloading:

Debug backtrace for WP_Block::render()
[11-Feb-2022 16:49:14 UTC] Array
(
    [0] => Array
        (
            [file] => /var/www/html/wordpress/wp/wp-includes/blocks.php
            [line] => 884
            [function] => render
            [class] => WP_Block
            [type] => ->
        )

    [1] => Array
        (
            [file] => /var/www/html/wordpress/wp/wp-includes/blocks.php
            [line] => 922
            [function] => render_block
        )

    [2] => Array
        (
            [file] => /var/www/html/wordpress/wp/wp-includes/class-wp-hook.php
            [line] => 307
            [function] => do_blocks
        )

    [3] => Array
        (
            [file] => /var/www/html/wordpress/wp/wp-includes/plugin.php
            [line] => 189
            [function] => apply_filters
            [class] => WP_Hook
            [type] => ->
        )

    [4] => Array
        (
            [file] => /var/www/html/wordpress/wp/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php
            [line] => 1811
            [function] => apply_filters
        )

    [5] => Array
        (
            [file] => /var/www/html/wordpress/wp/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php
            [line] => 560
            [function] => prepare_item_for_response
            [class] => WP_REST_Posts_Controller
            [type] => ->
        )

    [6] => Array
        (
            [file] => /var/www/html/wordpress/wp/wp-includes/rest-api/class-wp-rest-server.php
            [line] => 1141
            [function] => get_item
            [class] => WP_REST_Posts_Controller
            [type] => ->
        )

    [7] => Array
        (
            [file] => /var/www/html/wordpress/wp/wp-includes/rest-api/class-wp-rest-server.php
            [line] => 988
            [function] => respond_to_request
            [class] => WP_REST_Server
            [type] => ->
        )

    [8] => Array
        (
            [file] => /var/www/html/wordpress/wp/wp-includes/rest-api.php
            [line] => 511
            [function] => dispatch
            [class] => WP_REST_Server
            [type] => ->
        )

    [9] => Array
        (
            [file] => /var/www/html/wordpress/wp/wp-includes/rest-api.php
            [line] => 2860
            [function] => rest_do_request
        )

    [10] => Array
        (
            [function] => rest_preload_api_request
        )

    [11] => Array
        (
            [file] => /var/www/html/wordpress/wp/wp-includes/block-editor.php
            [line] => 492
            [function] => array_reduce
        )

    [12] => Array
        (
            [file] => /var/www/html/wordpress/wp/wp-admin/edit-form-blocks.php
            [line] => 72
            [function] => block_editor_rest_api_preload
        )

    [13] => Array
        (
            [file] => /var/www/html/wordpress/wp/wp-admin/post.php
            [line] => 187
            [args] => Array
                (
                    [0] => /var/www/html/wordpress/wp/wp-admin/edit-form-blocks.php
                )

            [function] => require
        )

)

Edit 2: A possible fix for this in WordPress/wordpress-develop#2312. I think this will speed-up the the editor for many users which have shortcodes or even blocks with complex frontend scripts.

@gziolo
Copy link
Member Author

gziolo commented Feb 11, 2022

A possible fix for this in WordPress/wordpress-develop#2312. I think this will speed-up the the editor for many users which have shortcodes or even blocks with complex frontend scripts.

Great debugging. I think that makes sense.

I initially thought we could tweak the rendering to avoid processing assets:

https://github.com/WordPress/wordpress-develop/blob/81cd3d58dcc5370910427f477e6a530b14aeb913/src/wp-includes/class-wp-block.php#L262-L272

However, what you proposed should have a similar result.

I think that this might be caused by the REST API preloading which is also rendering blocks and thus the render_callback is called which again enqueues all the assets. I have to do some more digging, but maybe this is already a known issue?

Still, it makes me wonder if we need to render blocks while preloading API requests? Do we need to send rendered HTML for blocks in the first place? Maybe we could look into some improvements in that aspect, too.

@ocean90
Copy link
Member

ocean90 commented Feb 11, 2022

I initially thought we could tweak the rendering to avoid processing assets:

I thought about that too but I didn't want to put any further logic into the render method which is most of the time not necessary.

Still, it makes me wonder if we need to render blocks while preloading API requests?

Some blocks may display the actual content of a post and then it would make sense to have the rendered content instead of some HTML markup. This also not only applies to the post editor but also to the site and widget editors.

I have moved this issue to Trac: https://core.trac.wordpress.org/ticket/55151

@ocean90
Copy link
Member

ocean90 commented Feb 14, 2022

register_block_script_handle() registers scripts by default to be enqueued in the header because the fifth argument is not set for wp_register_script() Does somebody know whether there was some discussion whether this should be changed to footer by default or allowed to be changed via block.json?

@jeremyfelt
Copy link
Member

Does somebody know whether there was some discussion whether this should be changed to footer by default or allowed to be changed via block.json?

I opened https://core.trac.wordpress.org/ticket/54018, which has a PR. I'm not sure if it's best as a ticket on Trac or issue here.

@gziolo
Copy link
Member Author

gziolo commented Feb 15, 2022

Does somebody know whether there was some discussion whether this should be changed to footer by default or allowed to be changed via block.json?

I opened https://core.trac.wordpress.org/ticket/54018, which has a PR. I'm not sure if it's best as a ticket on Trac or issue here.

Thank you for opening a ticket on Trac. Let's discuss the details there because we need to apply changes in WordPress core anyway. Once we have a working solution we can see if we can use hooks to bring the same functionality to the Gutenberg plugin.

I’m not aware of any prior discussion about changing the default flag in wp_register_script for the footer/header placement. It’s worth noting that @adamsilverstein has just opened a proposal to extend script loading strategies in WordPress/performance#168. Let’s make sure that the solution proposed takes that into account.

@dgwyer
Copy link
Contributor

dgwyer commented Feb 17, 2022

Just seeking clarification on how we currently define dependencies for styles and scripts in block.json. In the handbook it mentions how to specify an object to do this:

It’s possible to provide an object which takes the following shape:

handle (string) – the name of the script. If omitted, it will be auto-generated.
dependencies (string[]) – an array of registered script handles this script depends on. Default value: [].
version (string|false|null) – string specifying the script version number, if it has one, which is added to the URL as a query string for cache busting purposes. If the version is set to false, a version number is automatically added equal to current installed WordPress version. If set to null, no version is added. Default value: false.

This is related to supporting multiple styles and scripts in block.json but I'm not sure this has been implemented yet?

Also, are there plans to add support for "viewStyle" in block.json? This would allow you to add style definitions for the frontend only. I currently have a block which has certain markup that is displayed only on the frontend, so I need a way to add frontend styles.

@gziolo
Copy link
Member Author

gziolo commented Feb 18, 2022

This is related to supporting multiple styles and scripts in block.json but I'm not sure this has been implemented yet?

Actually, it's very good timing you asked this question. There is a following line in the docs that explains further the usage:

The definition is stored inside a separate PHP file which ends with .asset.php and is located next to the JS/CSS file listed in block.json. WordPress will automatically detect this file through pattern matching.

Now that we discuss support for multiple files per asset type and a way to provide some customization to how they are enqueued, @ocean90 proposed we could embed that configuration also inside block.json. More details in this comment https://core.trac.wordpress.org/ticket/54018#comment:5.

Also, are there plans to add support for "viewStyle" in block.json? This would allow you to add style definitions for the frontend only. I currently have a block which has certain markup that is displayed only on the frontend, so I need a way to add frontend styles.

There was no need so far so we ruled it out together with @aristath for v1. However, we'd love to hear more about the use case you have, @dgwyer. We assumed that styles are a bit different than scripts and you can achieve all goals throug style and customize the experience in the editor with editorStyle. Technically speaking, it is straightforward to add viewStyle if there are valid use cases.

@dgwyer
Copy link
Contributor

dgwyer commented Feb 18, 2022

Regarding array support for "editorScript", "script", "viewScript", "editorStyle", "style", and "viewStyle" (not currently implemented), will there a way to define dependencies via block.json too?

For example, for "viewScript" if I have a block of jQuery I want to run on the frontend then I'd need to also add "jQuery" as a dependency. At the moment there isn't a way to get this dependency into the generated .asset.php file. Dependencies should ideally be available for both JS, and CSS.


As for frontend only styles, I've come across the need for this on multiple occasions. Most recently I've been developing a simple slider block that doesn't render the final slider in the editor, it just allows you to add inner blocks to the slider block container. Only on the frontend do styles render the actual slider.

I had a similar case for a simple FAQ block too. Plus, there are other times where you may need to just add a few extra styles here and there, which are relevant to the frontend only. At the moment there is no way to separate out frontend styles only. It's a little restrictive at times.

@gziolo
Copy link
Member Author

gziolo commented Feb 19, 2022

Regarding array support for "editorScript", "script", "viewScript", "editorStyle", "style", and "viewStyle" (not currently implemented), will there a way to define dependencies via block.json too?

At the moment, there isn't a way to get this dependency into the generated .asset.php file.

It's a known limitation that we had to workaround in WordPress core, too:

https://github.com/WordPress/wordpress-develop/blob/c98f4c04db13a1c3db14345db4cff116dd01bf51/src/wp-includes/script-loader.php#L250-L260

Once we allow passing additional configuration for individual assets, having a way to provide a list of dependencies is a great idea, too. The only question would be whether we are fine redeclaring all dependencies manually, or we create a way to inject extra dependencies with some special entry like "..." for loaded deps :

"editorScript": [ {
    "path": "file:./index.js",
    "dependencies": [ "...", "jquery" ]
} ]

As for frontend only styles, I've come across the need for this on multiple occasions.

Thank you for sharing your examples. It's very helpful to learn about how people use the current API. I would love to see other people leaving their feedback as well. I'm also curious what @aristath thinks about it, as he spent a ton of time improving the existing handling for styles.

@jeremyfelt
Copy link
Member

I've had a hard time tracking down why viewScript is ignored when render_callback is used. Is that a necessary condition?

If so, I could see it being worth adding a note in the viewScript schema documentation and/or its area of the documentation rather than in the Frontend Enqueuing section (Which I missed for way too long today figuring this out 😅).

@gziolo
Copy link
Member Author

gziolo commented Feb 24, 2022

I've had a hard time tracking down why viewScript is ignored when render_callback is used. Is that a necessary condition?

It was modeled after two existing core blocks that enqueue viewScript conditionally in render_callback. It would be great to revisit now that plugins start using this API with their custom blocks.

render_callback for the File block:

$should_load_view_script = ! empty( $attributes['displayPreview'] ) && ! wp_script_is( 'wp-block-file-view' );
if ( $should_load_view_script ) {
wp_enqueue_script( 'wp-block-file-view' );
}

render_callback for the Navigation block"

$has_old_responsive_attribute = ! empty( $attributes['isResponsive'] ) && $attributes['isResponsive'];
$is_responsive_menu = isset( $attributes['overlayMenu'] ) && 'never' !== $attributes['overlayMenu'] || $has_old_responsive_attribute;
$should_load_view_script = ! wp_script_is( 'wp-block-navigation-view' ) && ( $is_responsive_menu || $attributes['openSubmenusOnClick'] || $attributes['showSubmenuIcon'] );
if ( $should_load_view_script ) {
wp_enqueue_script( 'wp-block-navigation-view' );
}

I'm curious whether we could define some strategies for loading viewScript in block.json as another config option in the proposal from @ocean90 in https://core.trac.wordpress.org/ticket/54018#comment:5, example:

"viewScript": [
    {
      "path": "file:./view.js",
      "strategy": "render_callback"
    }
]

This could be a way to give the control to render_callback for core blocks, but change the default always to enqueue the view scripts otherwise.

If so, I could see it being worth adding a note in the viewScript schema documentation and/or its area of the documentation rather than in the Frontend Enqueuing section (Which I missed for way too long today figuring this out 😅).

At least for the whole WordPress 5.9 cycle, we should improve the documentation to make it harder to miss.

@aristath
Copy link
Member

Using something like the examples described above would make sense...

"viewScript": [
    {
      "path": "file:./view.js",
      "strategy": "render_callback"
    }
]

^ That seems like a nice format, and one we can extend in the future.
path and handle would be the obvious default keys that we'd initially have to implement (handle in case we define the handle of an already loaded script instead of a path), and then we can add things like strategy etc.
Though tbh, I think strategy might not be the right key in this case... Thinking ahead, strategy for most people will mean something different (see this proposal on the performance repo), and we may want to implement an actual loading strategy in the future 😅

@pluginslab
Copy link

There's a good demand for having more flexibility in enqueueing scripts, for both the editor and view scripts. Specially, the footer options. WooCommerce Blocks requires in some cases that editor scripts are loaded at the footer.

CleanShot 2022-03-31 at 14 38 13@2x

So the solution for now is to go back to PHP and do it "the old way". If the idea is to make it possible so that blocks.json dictates how scripts are loaded ideally, we should add support for that like @ocean90 proposed. I really like that approach, it makes things very clear and easier to implement, I would say.

@gziolo
Copy link
Member Author

gziolo commented Aug 19, 2022

There's a good demand for having more flexibility in enqueueing scripts, for both the editor and view scripts. Specially, the footer options. WooCommerce Blocks requires in some cases that editor scripts are loaded at the footer.

@adamsilverstein, how is the work going on introducing strategies for loading scripts and styles? Once it's ready, I would be more than happy to look at integrating that with block assets in block.json file.

@gziolo
Copy link
Member Author

gziolo commented Aug 19, 2022

I have a patch WordPress/wordpress-develop#3108 ready targeting WordPress core that adds multiple assets support for individual script types in block.json: editorScript, script, and viewScript. It's also tracked in https://core.trac.wordpress.org/ticket/56408.

pento pushed a commit to WordPress/wordpress-develop that referenced this issue Sep 14, 2022
Follow-up #54337, [52069]. Part of WordPress/gutenberg#41236. More details in WordPress/gutenberg#33542.

Allow passing more than one script per block for `editorScript`, `script`, and `viewScript` fields in the `block.json` metadata file. This aligns with the previously added changes for `style` and `editorStyle` fields.

This change impacts the `WP_Block_Type` class and the REST API endpoint for block types. To ensure backward compatibiliy old names were soft deprecated in favor of new fields that work with array values and have `_handles` suffix.

Props zieladam, dlh, timothyblynjacobs, aristath, bernhard-reiter.
Fixes #56408.



git-svn-id: https://develop.svn.wordpress.org/trunk@54155 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this issue Sep 14, 2022
Follow-up #54337, [52069]. Part of WordPress/gutenberg#41236. More details in WordPress/gutenberg#33542.

Allow passing more than one script per block for `editorScript`, `script`, and `viewScript` fields in the `block.json` metadata file. This aligns with the previously added changes for `style` and `editorStyle` fields.

This change impacts the `WP_Block_Type` class and the REST API endpoint for block types. To ensure backward compatibiliy old names were soft deprecated in favor of new fields that work with array values and have `_handles` suffix.

Props zieladam, dlh, timothyblynjacobs, aristath, bernhard-reiter.
Fixes #56408.


Built from https://develop.svn.wordpress.org/trunk@54155


git-svn-id: http://core.svn.wordpress.org/trunk@53714 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to platformsh/wordpress-performance that referenced this issue Sep 14, 2022
Follow-up #54337, [52069]. Part of WordPress/gutenberg#41236. More details in WordPress/gutenberg#33542.

Allow passing more than one script per block for `editorScript`, `script`, and `viewScript` fields in the `block.json` metadata file. This aligns with the previously added changes for `style` and `editorStyle` fields.

This change impacts the `WP_Block_Type` class and the REST API endpoint for block types. To ensure backward compatibiliy old names were soft deprecated in favor of new fields that work with array values and have `_handles` suffix.

Props zieladam, dlh, timothyblynjacobs, aristath, bernhard-reiter.
Fixes #56408.


Built from https://develop.svn.wordpress.org/trunk@54155


git-svn-id: https://core.svn.wordpress.org/trunk@53714 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@gziolo
Copy link
Member Author

gziolo commented Sep 14, 2022

Now that Allow registering multiple items for all supported asset types landed, we have achieved a good level of consistency. The only remaining task now is to document all changes and compile a dev note.

Add viewStyle - frontend only multiple styles support for the block type

I consider this one nice to have, and it's being tracked in #41236, so I'm going to close this issue as resolved.

@gziolo gziolo closed this as completed Sep 14, 2022
@gziolo gziolo removed the [Status] In Progress Tracking issues with work in progress label Sep 14, 2022
whereiscodedude pushed a commit to whereiscodedude/wpss that referenced this issue Sep 18, 2022
Follow-up #54337, [52069]. Part of WordPress/gutenberg#41236. More details in WordPress/gutenberg#33542.

Allow passing more than one script per block for `editorScript`, `script`, and `viewScript` fields in the `block.json` metadata file. This aligns with the previously added changes for `style` and `editorStyle` fields.

This change impacts the `WP_Block_Type` class and the REST API endpoint for block types. To ensure backward compatibiliy old names were soft deprecated in favor of new fields that work with array values and have `_handles` suffix.

Props zieladam, dlh, timothyblynjacobs, aristath, bernhard-reiter.
Fixes #56408.


Built from https://develop.svn.wordpress.org/trunk@54155
ootwch pushed a commit to ootwch/wordpress-develop that referenced this issue Nov 4, 2022
Follow-up #54337, [52069]. Part of WordPress/gutenberg#41236. More details in WordPress/gutenberg#33542.

Allow passing more than one script per block for `editorScript`, `script`, and `viewScript` fields in the `block.json` metadata file. This aligns with the previously added changes for `style` and `editorStyle` fields.

This change impacts the `WP_Block_Type` class and the REST API endpoint for block types. To ensure backward compatibiliy old names were soft deprecated in favor of new fields that work with array values and have `_handles` suffix.

Props zieladam, dlh, timothyblynjacobs, aristath, bernhard-reiter.
Fixes #56408.



git-svn-id: https://develop.svn.wordpress.org/trunk@54155 602fd350-edb4-49c9-b593-d223f7449a82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Package] Block editor /packages/block-editor [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues.
Projects
None yet
Development

No branches or pull requests

9 participants