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: Allow customizations for assets in block.json #46954

Open
jakeparis opened this issue Jan 6, 2023 · 10 comments
Open

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

jakeparis opened this issue Jan 6, 2023 · 10 comments
Labels
[Feature] Block API API that allows to express the block paradigm. Needs Dev Ready for, and needs developer efforts [Type] Enhancement A suggestion for improvement.

Comments

@jakeparis
Copy link
Contributor

jakeparis commented Jan 6, 2023

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: #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" ]
@jakeparis jakeparis changed the title I would like to be able to declare a script dependency in block.json's "editorScript" or "viewScript" I would like to be able to declare a script dependency when registering it in block.json' Jan 6, 2023
@jakeparis jakeparis changed the title I would like to be able to declare a script dependency when registering it in block.json' I would like to be able to declare a script dependency when registering it in block.json Jan 6, 2023
@talldan talldan added [Feature] Block API API that allows to express the block paradigm. Needs Technical Feedback Needs testing from a developer perspective. [Type] Enhancement A suggestion for improvement. labels Jan 9, 2023
@gziolo gziolo mentioned this issue Jan 30, 2023
58 tasks
@gziolo gziolo changed the title I would like to be able to declare a script dependency when registering it in block.json Blocks: Allow customizations for assets in block.json Jan 30, 2023
@gziolo
Copy link
Member

gziolo commented Jan 30, 2023

@jakeparis, thank you for opening this proposal. Actually, it's something that has been on the Block API roadmap #41236 for quite some time, but it didn't have a related issue. We had a related issue #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

@gziolo gziolo added the Needs Dev Ready for, and needs developer efforts label Jan 30, 2023
@gziolo gziolo removed the Needs Technical Feedback Needs testing from a developer perspective. label Feb 26, 2023
@lgladdy
Copy link

lgladdy commented Dec 18, 2023

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
Copy link
Member

gziolo commented Dec 20, 2023

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 #57234. Let’s remove this limitation if it doesn’t work great with custom blocks.

@lgladdy
Copy link

lgladdy commented Dec 21, 2023

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

@gziolo
Copy link
Member

gziolo commented Dec 21, 2023

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.

@lgladdy
Copy link

lgladdy commented Dec 21, 2023

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 #57234 (comment) 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
Copy link
Member

I left some follow-up on #57234 (comment)

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

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.

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
Copy link
Member

gziolo commented Jan 3, 2024

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: WordPress/wordpress-develop#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:

  • strategy string – Optional. If provided, may be either 'defer' or 'async'.
  • in_footer bool – Optional. Whether to print the script in the footer. Default 'false'.

@colorful-tones
Copy link
Member

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
Copy link
Contributor Author

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.

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. Needs Dev Ready for, and needs developer efforts [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

5 participants