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 reading the script handle from asset files #5118

Closed
wants to merge 1 commit into from

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Aug 30, 2023


Trac ticket: https://core.trac.wordpress.org/ticket/60485

In the documentation for WPDefinedAsset definition for block metadata there is a note about handle:

handle (string) – the name of the script. If omitted, it will be auto-generated.

We never implemented it, but it would be very useful for block authors. One of the limitations of the way scripts get registered for blocks is that whenever a single file needs to be shared between blocks, then it has to be registered using wp_register_script separately, and the script handle used needs to be passed to one of the script fields in block.json: editorScript, script or viewScript. It would be great if we could pass the predictable script handle in the .asset.php file so you could still use local paths in block.json and the rest would get automatically handled by WordPress core. In effect, the logic proposed would read the script handle from the asset file and ensure that all other occurrences reuse the script that is already registered under the same name. It would open the possibility to have firs-class support for shared chunks or runtimes generated by bundlers like webpack.

My initial goal was to find a way to enable runtime chunk for @wordpress/scripts in development mode and automatically register it in WordPress core to ensure that React Fast Refresh works correctly with more than one block as summarized in WordPress/gutenberg#25188 (comment).

However, I figured out that we could also use the exact mechanism with any shared chunk that @jsnajdr made possible in the first place with WordPress/gutenberg#41002 by improving asset file generation in the webpack plugin that handles dependencies for WP core.


Note: This work, if we agree it's the good direction, is going to require follow-up changes (not a blocker for this patch) to the @wordpress/dependency-extraction-webpack-plugin, example build/shared-script.js:

<?php

return array(
	'handle'       => 'my-plugin-shared-script',
	'dependencies' => array(),
	'version'      => 'test',
);

My initial thinking is that we can auto-generate the script handle based on the combination of the name of the webpack config or the project, plus the sanitized path of the entry file relative to the build folder:

'my-plugin' + '-' + 'shared-script'


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@jsnajdr
Copy link
Member

jsnajdr commented Aug 31, 2023

Yes, this looks like a good feature: instead of always auto-generating script handle for a block, like:

jetpack/ai-assistant block + editorScript field => jetpack-ai-assistant-editor-script handle

we can declare a custom name for the handle in the .asset.php file.

I suppose the use case would be as follows below. Please confirm whether I understand it correctly.

There's a plugin that registers three blocks, and wants to bundle all their scripts together. The file layout is like:

block-one/block.json
block-two/block.json
block-three/block.json
edit.js
edit.asset.php

All the three block.json files have the same editorScript field, pointing to the same edit.js file:

"editorScript": "file:../edit.js"

The edit.js file is a bundle of scripts for all three blocks, and it has three registerBlockType( 'my-plugin/*', ... ) calls, one for each block.

The edit.asset.php specifies a custom handle, e.g., my-plugin-blocks-editor-script. This handle is common for all three blocks, i.e., a WP_Block_Type_Registry::get_instance()->get_registered( 'my-plugin/*' ) will return the same editor_script_handles for all three:

{
  editor_script_handles: [ 'my-plugin-blocks-editor-script' ]
}

If we didn't have the custom handle in edit.asset.php, the same edit.js script would get registered under three different auto-generated handles:

my-plugin-one-editor-script
my-plugin-two-editor-script
my-plugin-three-editor-script

and would be enqueued and loaded three times.

I can confirm that this is compatible with the async block loading prototype. As long as the registered block specifies all the asset handles correctly, they can be used to create the import map and loaded dynamically in the browser.


How to build such "block libraries" with webpack and wp-scripts? I think it could be automated. In the source code, each block would reference only its own edit script:

"editorScript": "file:./init.js"

Then the task for the build script would be: "collect all blocks in ./src/blocks/*/block.json and bundle them into one block library." The build script would loop through all the block.json files, collect all editorScript fields there and would bundle them together, outputting the directory structure described above. The file:./init.js references in the source would be changed to file:../edit.min.js in the build directory.

One thing I don't like about the block.json files in #51778 is that the script references point to the built files. There is a field like:

"editorScript": "file:./editor.min.js"

in the source block.json although the editor.min.js file doesn't exist at all in the source. You need to guess the production file name. I think that these references should point to source files, and should be transformed during the build.

@gziolo
Copy link
Member Author

gziolo commented Oct 15, 2023

I suppose the use case would be as follows below. Please confirm whether I understand it correctly.

There's a plugin that registers three blocks, and wants to bundle all their scripts together. The file layout is like:

block-one/block.json
block-two/block.json
block-three/block.json
edit.js
edit.asset.php

All the three block.json files have the same editorScript field, pointing to the same edit.js file:

"editorScript": "file:../edit.js"

The edit.js file is a bundle of scripts for all three blocks, and it has three registerBlockType( 'my-plugin/*', ... ) calls, one for each block.

The edit.asset.php specifies a custom handle, e.g., my-plugin-blocks-editor-script. This handle is common for all three blocks, i.e., a WP_Block_Type_Registry::get_instance()->get_registered( 'my-plugin/*' ) will return the same editor_script_handles for all three:

{
  editor_script_handles: [ 'my-plugin-blocks-editor-script' ]
}

If we didn't have the custom handle in edit.asset.php, the same edit.js script would get registered under three different auto-generated handles:

my-plugin-one-editor-script
my-plugin-two-editor-script
my-plugin-three-editor-script

and would be enqueued and loaded three times.

This is an excellent summary what type of problems I’m trying to attack. I must admit that I didn’t think about the obvious and so popular scenario described where all editor files for individual blocks get bundled and enqueued together in the plugin. However, it’s an even better example to illustrate the limitations of the current implementation of scripts handling during block registration. I was thinking more about the cases where multiple blocks need to enqueue the same script, or when instead of bundling all blocks together, the runtime feature is used as a way to share code together with some vendor libraries.

How to build such "block libraries" with webpack and wp-scripts? I think it could be automated. In the source code, each block would reference only its own edit script:

"editorScript": "file:./init.js"

Then the task for the build script would be: "collect all blocks in ./src/blocks/*/block.json and bundle them into one block library." The build script would loop through all the block.json files, collect all editorScript fields there and would bundle them together, outputting the directory structure described above. The file:./init.js references in the source would be changed to file:../edit.min.js in the build directory.

That would be very convenient to give more power to wp-scripts allowing rewriting of block.json files to further improve the developer experience.

I’m not sure if in all cases bundling editor scripts together is the expected intent though, but it would align with how the plugins often structure JavaScript code for blocks in the editor. What benefits you though of when describing the idea of bundling together several editScript scripts? How does it differ from using the runtime or code splitting features from weback?

It should be a different story when talking about the front end though as we should rather aim to split the JavaScript as much as possible to ensure that it is only enqueued when essential for the block to work.

One thing I don't like about the block.json files in #51778 is that the script references point to the built files. There is a field like:

"editorScript": "file:./editor.min.js"

in the source block.json although the editor.min.js file doesn't exist at all in the source. You need to guess the production file name. I think that these references should point to source files, and should be transformed during the build.

That's a great point and I fully agree that it's confusing, counterintuitive and far from what I'd like to see. The challenge is that you need to explain the idea of crafting the block.json file to two different groups of plugin authors: these using wp-scripts that should be referencing source files and these who use their development pipeline that should target the production version of scripts and styles. I would love us to figure out solutions to the documentation challenge and offer a better integration with wp-scripts as it feels like the developer should only think about the structure of the source folder and the rest should magically happen inside the tool. I hope we can get there seperately from the idea covered in this PR.

@gziolo
Copy link
Member Author

gziolo commented Jan 3, 2024

Related discussion on WordPress Slack where shared scripts would resolve the existing issue:

I have a plugin that registers a custom wordpress/data data store, and two distinct blocks. Both of those blocks use the data store. However, the default build process appears to include the data store in the built JS for both blocks, leading to console errors on the frontend of Store "xxxxx" is already registered. Presumably my data store should be compiled into its own build output and just marked as a dependency for the blocks, but not sure what I should be looking at to accomplish that?

There is a very similar issue reported with how @wordpress/scripts works when it builds parent and child blocks that share the same React context. When every block has an entry point that bundles the copy of the same context, it doesn’t work as expected.

@gziolo gziolo force-pushed the update/shared-block-scripts branch from 038ff22 to df35a2e Compare February 9, 2024 11:21
@gziolo gziolo marked this pull request as ready for review February 9, 2024 11:29
Copy link

github-actions bot commented Feb 9, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props gziolo, jsnajdr, youknowriad.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link

github-actions bot commented Feb 9, 2024

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@youknowriad
Copy link
Contributor

The reasoning makes sense. I wanted to mention that we could potentially be smarter and check whether multiple blocks refer to the same file even If the handle is not specified and just use the same generated or uuid handle as well.

@youknowriad
Copy link
Contributor

@jsnajdr The remark about the build changing the path from "source" to "built" is relevant but I'd also compare it to "package.json".

In "package.json", you don't define a path the "source" file, you only define a path to the "built" files (main, module, exports), so while it's indeed a bit confusing, I think not targeting the source file can be a good thing because you don't really know how the "source" file is written: is it typescript, it it jsx, .... so unless it's normalized, it's not entirely clear why you should point to it from a declarative file like block.json.

@gziolo
Copy link
Member Author

gziolo commented Feb 12, 2024

I wanted to mention that we could potentially be smarter and check whether multiple blocks refer to the same file even If the handle is not specified and just use the same generated or uuid handle as well.

I will explore that further as a follow-up, as that would be indeed a nice addition. The biggest question for me is how much processing would be required to detect the same paths because scripts are stored with handles as keys.

The way how to handle block.json integration with @wordpress/scripts is a more nuanced topic that deserves its issue. The package.json analogy is interesting. The current state of the art is that we do some light processing on the paths provided targeting the final path like file:index.js to account for the fact that the file might be using TS or JSX. In effect, if the file is inside src/block/index.ts the build script will still figure it out and end up including build/block/index.js. Anyway, that's the separate question where we want to go from here in the future.

@gziolo
Copy link
Member Author

gziolo commented Feb 12, 2024

Committed with https://core.trac.wordpress.org/changeset/57590.

@gziolo gziolo closed this Feb 12, 2024
@gziolo gziolo deleted the update/shared-block-scripts branch February 12, 2024 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants