WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.5k stars 4.19k forks source link

Blocks: Allow customizations for assets in `block.json` #46954

Open jakeparis opened 1 year ago

jakeparis commented 1 year ago

What problem does this address?

Part of #41236.

When registering a script via block.json, there is no way to specify a dependency. I understand that webpack uses a script to create index.asset.php with wordpress dependencies, but that doesn't work for other scripts. Our organization has some wordpress scripts which are core to our experience and relied on by other blocks/plugins. When I use block.json to register my block like "editorScript": [ "file:./index.js" ],, there's no way to declare those core scripts as dependencies. And doing it this way: "editorScript": [ "my-core-script", "file:./index.js" ], doesn't work either.

See this discussion also: https://github.com/WordPress/gutenberg/discussions/46589

What is your proposed solution?

Allow the existing WPDefinedAsset shape of input to be used along with the filepath method of registered a script. It might look like this:


"editorScript": [
  {
    "src": "file:./index.js",
    "dependencies": [  "my-core-script" ],
    "version": false,
  }
],
// could also still use current method
"viewScript": [ "file:./index.j" ]
gziolo commented 1 year ago

@jakeparis, thank you for opening this proposal. Actually, it's something that has been on the Block API roadmap https://github.com/WordPress/gutenberg/issues/41236 for quite some time, but it didn't have a related issue. We had a related issue https://github.com/WordPress/gutenberg/issues/40447 that would get fixed as a consequence of implementing the syntax you outlined.

What's very interesting here is that nearly the same proposal was shared in https://core.trac.wordpress.org/ticket/54018#comment:5 by @ocean90. So that confirms that we were heading in the right direction to address the existing use cases.

Screenshot 2023-01-30 at 16 17 14
lgladdy commented 10 months ago

Big fan of this PR, ACF Blocks users often are confused by the requirement for an .asset.php file to exist, especially as the documentation says "you may" but it will throw a _doing_it_wrong if it doesn't exist.

gziolo commented 10 months ago

Big fan of this PR, ACF Blocks users often are confused by the requirement for an .asset.php file to exist, especially as the documentation says "you may" but it will throw a _doing_it_wrong if it doesn't exist.

I see a relevant issue open https://github.com/WordPress/gutenberg/issues/57234. Let’s remove this limitation if it doesn’t work great with custom blocks.

lgladdy commented 10 months ago

@gziolo Sounds good to me! I think with both these PRs merged we'll have covered off most use cases!

gziolo commented 10 months ago

The other idea I shared in https://github.com/WordPress/gutenberg/issues/57234#issuecomment-1866091936, that instead of tweaking the format of block.json, 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.

lgladdy commented 10 months ago

The other idea I shared in #57234 (comment), that instead of tweaking the format of block.json, 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.

Yeh, that new idea in https://github.com/WordPress/gutenberg/issues/57234#issuecomment-1866091936 would work too, and arguably is better than having the filter lower down the stack, so long as it prevents checking for the asset file completely if all the data is available at that point.

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.

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.

colorful-tones commented 10 months ago

I left some follow-up on https://github.com/WordPress/gutenberg/issues/57234#issuecomment-1866530992

Overall, I think extending the block.json to allow developers to customize asset definitions would be beneficial.

The other idea I shared in https://github.com/WordPress/gutenberg/issues/57234#issuecomment-1866091936, that instead of tweaking the format of block.json, 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.

Would this even be necessary or a hindrance to exploring extending the definitions in block.json? If it is not entirely necessary then I would encourage us to consider logging a separate Issue and PR to explore whether developers would prefer this extra PHP way.

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. 🤔

I love this added performance consideration and impact, and great callout @lgladdy

gziolo commented 10 months ago

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.

I was thinking about it a bit more and I figured out that we could also combine two worlds together and optimize the way wp-scripts build work today where you put for example in the src folder the view script referenced in the block.json:

{
    "viewScript": [ "file:./index.js" ]
}

Instead of creating the asset file on the disk, with the proposal implemented maybe we would be able to inline it in the block.json copied to the build folder, so the stuff from above gets transformed to:

{
    "viewScript": [ {
        "file": "./index.js",
        "dependencies": [],
        "version": "abc123"
    } ]
}

For start, let's agree on the shape of the object passed to the block.json file. The asset file currently contains the following shape documented as WPDefinedAsset:

  • 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.

In practice, handle was never implemented and integrated in WordPress core, but I have PR opened that would address it at least on the WP core side: https://github.com/WordPress/wordpress-develop/pull/5118.

Is it enough for start or should it be also possible to support other arguments that can be passed to the [wp_register_script] call like:

colorful-tones commented 10 months ago

Instead of creating the asset file on the disk, with the proposal implemented maybe we would be able to inline it in the block.json copied to the build folder, so the stuff from above gets transformed to:

This makes sense to me. This would be a modification to the @wordpress/create-block package only, correct? @gziolo

For start, let's agree on the shape of the object passed to the block.json file.

I think handle, dependencies, version, strategy and in_footer would be a great start. 🥳

jakeparis commented 10 months ago

Is it enough for start or should it be also possible to support other arguments that can be passed to the [wp_register_script] call

If at all possible, the other arguments available to wp_register_script() should be available here as well, for the sake of consistency. So, I agree with the previous comment: handle, dependencies, version, strategy and in_footer would be great.

ArrayIterator commented 5 months ago

Is it enough for start or should it be also possible to support other arguments that can be passed to the [wp_register_script] call

If at all possible, the other arguments available to wp_register_script() should be available here as well, for the sake of consistency. So, I agree with the previous comment: handle, dependencies, version, strategy and in_footer would be great.

I agreed! On some case we don't need xxxx.asset.php again! Because we have been declare about : block.json declare title icon etc. and then on editor (js) script, we also need a registerType() with the name, icon, category etc. So? we should declare multiple files that should matches each others! need 3 files for registering block via json: block.json, asset.php & editor.js