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

Consider filter for allowing block asset registration index.asset.php optional #57234

Closed
colorful-tones opened this issue Dec 19, 2023 · 12 comments
Labels
[Feature] Block API API that allows to express the block paradigm. [Type] Enhancement A suggestion for improvement.

Comments

@colorful-tones
Copy link
Member

What problem does this address?

As previously discussed in #40447, it is a hard requirement that a *.asset.php file is present to register a block in WordPress. Otherwise, it throws a Warning: function register_block_script_handle was called incorrectly. The asset file (/wp-content/uploads/faustwp/blocks/block-a/index.asset.php) for the "editorScript" defined in "create-block/block-a" block definition is missing. (This message was added in version 5.5.0.), and the block will not appear in the inserter.

Decoupled applications would benefit from allowing this condition as optional.

Leaning into the rich history of WordPress hooks and filters, it could become optional for this file to exist, opening up a world of possibilities in decoupled architecture. Please see Faust.js's example: wpengine/faustjs#1656.

This would also allow developers flexibility in tooling and possibly set up custom webpack integrations (or even other build tools). Similarly, this may benefit progress toward the experiments in lazy-loading block infrastructure #2768.

What is your proposed solution?

One solution: allow filtering of $script_asset_path, as seen in this proof-of-concept PR from @josephfusco

WordPress/wordpress-develop#5799

@colorful-tones colorful-tones added [Type] Enhancement A suggestion for improvement. [Feature] Block API API that allows to express the block paradigm. labels Dec 19, 2023
@colorful-tones
Copy link
Member Author

Tagging @gziolo as he likely has some insight on the overall vision and progress being made on related work here: #41236 🙏

@gziolo
Copy link
Member

gziolo commented Dec 21, 2023

We are discussing in #46954 (comment) removing the check to enforce the .asset.php file. Maybe it should be some sort of developer prompt that encourages using it as the best practice. I am curious to know how developer-friendly that would be, though.

Instead, maybe we should put a filter just before registering the block related script to allow developers to apply their modifications before this code:
https://github.com/WordPress/wordpress-develop/blob/77dcb1771ae14e655edaf514f0f55c28212d9bb2/src/wp-includes/blocks.php#L190-L196

What do you think?

@colorful-tones
Copy link
Member Author

We are discussing in #46954 (comment) removing the check to enforce the .asset.php file.

👍 🤔

Maybe it should be some sort of developer prompt that encourages using it as the best practice. I am curious to know how developer-friendly that would be, though.

I'm not sure what the benefit would be. It would seem to just a be courtesy that could lead to confusion. 🤔

Instead, maybe we should put a filter just before registering the block related script to allow developers to apply their modifications before this code:
https://github.com/WordPress/wordpress-develop/blob/77dcb1771ae14e655edaf514f0f55c28212d9bb2/src/wp-includes/blocks.php#L190-L196
What do you think?

Accounting for the input and considerations in #46954 – I'm trying to distinguish where there is inclusivity or exclusivity in each feature request.

I like the idea of allowing developers to opt out of the required .asset.php. I also love the idea of allowing developers the ability to customize their assets in the block.json and extending the overall definitions allowed within.

As @lgladdy expressed #46954 (comment):

The thing I like about this PR is that it would allow you to build a simple block, regardless of type, without having to create one additional file (and indeed, one additional filesystem read - which can be performance-expensive on value hosting), whereas the filter would require additional code to make it work.

This is a benefit I had not considered. 👍

Also, considering @lgladdy:

I think it's reasonable that if block.json contains the file path for the WPDefinedAsset, it should also be possible for the block.json to provide the attributes for that file.

And @gziolo offering:

we could put a filter that allows modifying all the params that get passed to wp_register_script with the context of the block metadata available so plugins can apply all the necessary modifications using the full power of PHP and WP core.

I'm not sure that the extra step would be necessary? Could it potentially be a follow-up Issue and Pull Request, or would it be required as part of the work to get #46954 resolved? It might take some code exploration.

@colorful-tones
Copy link
Member Author

Just a heads up - I will be off for the rest of the year. I look forward to the momentum of 2024. ❤️

@josephfusco
Copy link

josephfusco commented Jan 2, 2024

... and the block will not appear in the inserter.

For clarity, when registering decoupled blocks via Faust.js, the blocks are correctly appearing/functioning in the inserter. This can be seen in the project's block-support example.

I've outlined some potential solutions that would work for my use case; removing what I believe to be an unnecessary warning that does not account for all block registration scenarios.

  1. Remove the _doing_it_wrong warning
  2. Add a new filter to omit the problematic warning (Editor: Make asset registration .asset.php optional for blocks wordpress-develop#5799)
  3. Block.json asset customization Blocks: Allow customizations for assets in block.json #46954

@gziolo
Copy link
Member

gziolo commented Jan 3, 2024

After reading the comments, I'm in favor of removing the _doing_it_wrong warning and continuing the script registration process with the default values provided. So it would be similar to what @josephfusco proposed in WordPress/wordpress-develop#5799 but without the filter, as we are essentially proceeding as it was always set to false. In retrospect, I don't think we should force everyone to use wp-scripts build to generate this file if they want to set the file path. If I recall correctly, the initial thinking was that folks could register the script themselves and put the script handle instead, which is more work. I left a comment on the core patch, @josephfusco do you plan to work on bringing it to the finish line?

Let's also work on the block.json asset customization separately.

@fabiankaegy
Copy link
Member

I think the description of the ticket here is somewhat misleading. The error only happens when a block tries to register any asset via a local file: path. In which case WordPress is now responsible for registering and enqueueing this asset. And I think it is reasonable for there to be the requirement for the .asset.php file because if you don't provide that the script won't have a cache busting version (maybe this is not 100% true when the block.json file has a version number) nor is there a way to manage dependencies. (This is true for script, editorScript, viewScript, style, editorStyle)

{
    "viewScript": "file:./view.js"
}

If you don't want that handling, it is not a requirement for building & registering blocks.

Instead you can always either completely manually handle all the asset registration and enqueueing without even making WordPress aware of it. Or you can register all your assets and then pass the handles to the various properties in the block.json file which will also not trigger this error because WordPress is not in charge of handling the registration of the asset.

wp_register_script(
    'my-custom-handle',
    $url,
    $dependencies,
    $version,
    [ 'strategy' => 'defer' ]
);
{
    "apiVersion": 3,
    "viewScript": "my-custom-handle"
}

Having said all that I'm not strictly against adding some more / better handlig for cases where the .asset.php file doesn't exist. But I do think it is very reasonable for WordPress to expect it to be there in the default case and there are many ways to not run into it when you don't want the automatic registration.

And also yeah I'm 100% still in favor of adding more options to block.json to further customize asset registration as tracked in #46954 :)

@colorful-tones
Copy link
Member Author

I think the description of the ticket here is somewhat misleading. The error only happens when a block tries to register any asset via a local file: path. In which case WordPress is now responsible for registering and enqueueing this asset. And I think it is reasonable for there to be the requirement for the .asset.php file because if you don't provide that the script won't have a cache busting version (maybe this is not 100% true when the block.json file has a version number) nor is there a way to manage dependencies. (This is true for script, editorScript, viewScript, style, editorStyle)

@fabiankaegy thanks for the additional clarity. 👍

And also yeah I'm 100% still in favor of adding more options to block.json to further customize asset registration as tracked in #46954 :)

Indeed. 😄

@josephfusco
Copy link

PR has been updated! :octocat: WordPress/wordpress-develop#5799

@colorful-tones
Copy link
Member Author

@gziolo do you think this is ready for merge? Need more feedback?

@gziolo
Copy link
Member

gziolo commented Jan 26, 2024

do you think this is ready for merge? Need more feedback?

Yes, it’s almost ready. I have enough to land it. I need to find some time next week for it 👍

@gziolo
Copy link
Member

gziolo commented Feb 8, 2024

This is now resolved in WordPress core with WordPress/wordpress-develop@0ff0842.

@gziolo gziolo closed this as completed Feb 8, 2024
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. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

4 participants