Build Tooling: Generate asset file in PHP format with the list dependencies #17298
Conversation
gziolo
added
[Type] Enhancement
|
It's unclear to me what the purpose is behind doing this. Is it for performance reasons so that php runtime doesn't have to decode json from the original |
|
I don't think this would improve performance since it is using I ended up doing exactly this by making a simple PHP script to dump JSON input as PHP. |
I'm seeking a way to make assets registration much easier and more automated when you use wp_register_script( string $handle, string|bool $src, array $deps = array(), string|bool|null $ver = false, bool $in_footer = false )https://developer.wordpress.org/reference/functions/wp_register_script/ and wp_register_style( string $handle, string|bool $src, array $deps = array(), string|bool|null $ver = false, string $media = 'all' )https://developer.wordpress.org/reference/functions/wp_register_style/ They both are very similar and we could further simplify them in some way with a wrapper or by changing its internal implementation: wp_register_asset( 'my/path/to/script.js' );where WordPress would load the file with There are a couple of considerations here:
I'm fine with doing some benchmarks to find the best possible format which core and plugins would consume. @sirreal, did you verify it when integrating with Jetpack. I think we discussed that PHP format would work better here. We could try also to find a way to generate a PHP file which returns an array but there is this challenge how to make it secure. |
Yes, that would be the best possible solution. Do you know how to do the same trick using Node.js? :D |
There was a problem hiding this comment.
Ahh that explanation makes things much clearer 👍.
On the wp core side of things, we'd probably want an api like this:
function wp_register_asset( $asset_path, $handle = '', $dependencies = [], $asset_config_path = '' ) {
/**
* If `$asset_config_path` is empty, then try deriving the `*.asset.php` configuration file from the `$asset_path` value.
* If `$handle` is empty, then generate from `$asset_path`.
* `$dependencies` would still be needed in case there are additional dependencies that don't get picked up via build process.
**/
}| .digest( 'hex' ), | ||
| } ) | ||
| .replace( /\.js$/i, '.deps.json' ); | ||
| .replace( /\.js$/i, '.asset.' + outputFormat ); |
There was a problem hiding this comment.
This will be a breaking change for any code relying on deps.json currently right?
There was a problem hiding this comment.
Yes, totally. We could make it backward compatible with some flag but I think for CLI tools it's fine to introduce breaking changes.
What did was call the PHP executable from node. I found a library at one point for generating the PHP in nodejs, but I wasn't very confident in it. And I'm now of course completely failing to find the name of it. |
You would need to have PHP installed to make it work. Ideally, it's all Node-based. |
|
Might need to generate the php similar to how the |
|
This was the library, https://github.com/aliaksandr-master/grunt-json2php |
I recall discussing PHP as the output format and speculation that it might have better performance, but I haven't done any benchmarking and don't expect to in the near future. Jetpack is using the JSON serialized output. There was some related conversation here: #15124 (comment) |
|
@aristath shared this nice npm package https://www.npmjs.com/package/json2php on WordPress Slack: I used it in d0a983f.
@TimothyBJacobs - this library is nearly identical but it has more cases in the switch statement :)They use this trick in this grunt lib where they serialize to JSON and parse back to JS object which I applied as well to increase the likeliness that it would work properly with some code added through the Webpack plugin advanced options. Example output:
<?php return array('dependencies' => array('lodash', 'wp-a11y', 'wp-api-fetch', 'wp-block-editor', 'wp-block-library', 'wp-blocks', 'wp-components', 'wp-compose', 'wp-core-data', 'wp-data', 'wp-editor', 'wp-element', 'wp-hooks', 'wp-i18n', 'wp-keycodes', 'wp-media-utils', 'wp-notices', 'wp-nux', 'wp-plugins', 'wp-polyfill', 'wp-url', 'wp-viewport'), 'version' => '8ccda406e48392e355b4551433d635a1'); |
Yes, it would be great to have it automated for all parties involved: Gutenberg, core, and plugins. |
|
@swissspidy - do you think it would make sense to put a flag in this asset file which indicates whether this file contains translation? At the moment in WordPress core, we have a hardcoded list of scripts which contain translations: It would be great to be able to automate it as well :) |
I did some initial coding and I think this PR is big enough. Let's tackle it separately. I will update documentation, add CHANGELOG entries and we can land code changes in the current shape. I want to make sure it's included in the next npm release so we can simplify packages handling in WordPress core. At the moment the biggest struggle is the necessity to keep versions and lists of dependencies for each package up to date in PHP files. |
gziolo
force-pushed
the
update/webpack-asset-php
branch
from
237464e to
4a1843f
Compare
Co-Authored-By: Jon Surrell <[email protected]>
Co-Authored-By: Jon Surrell <[email protected]>
gziolo
force-pushed
the
update/webpack-asset-php
branch
from
ad87d77 to
a0dd11b
Compare
|
This is my script I embedded in Gutenberg to test it: $asset_file = substr( $path, 0, -3 ) . '.asset.json';
$time_json = microtime( true );
for ($i = 0; $i < 100; $i++) {
$asset = file_exists( $asset_file )
? json_decode( file_get_contents( $asset_file ) )
: null;
}
$execution_time = microtime( true ) - $time_json;
print $asset_file . ': ' . $execution_time . '<br />';
$asset_file = substr( $path, 0, -3 ) . '.asset.php';
$time_php = microtime( true );
for ($i = 0; $i < 100; $i++) {
$asset = file_exists( $asset_file )
? require( $asset_file )
: null;
}
$execution_time = microtime( true ) - $time_php;Here are results: I didn't do in depth analysis as it's evident that PHP version is on average 10 times faster! |
|
Thank you all for all valuable feedback, commits, suggestions and making it happen :) Let's iterate further as noted in #17454. |
|
I forgot to update CHANGELOG files yesterday. I'll try to do it today :) It's referencing I can see changes from this PR published in: |
|
The benchmark above is flawed afaict, as, at best, the PHP file is only read once (and then stays in the OPCache), and at worst, it is never read at all because you ran the test before and it was already in the cache. So, in essence, you are testing a cached call against an uncached call. Of course PHP will be faster here, but that doesn't mean you couldn't see the similar performance with cached JSON. But more importantly, was this ever a bottleneck? It seems to me that an optimization is used here that can lead to all sorts of misunderstandings, and maybe even security incidents, as security decisions are often based on folder names and their assumed content. In a plugin I'm working on, for example, this now causes PHP files to reside under |
|
@schlessera - thanks for sharing your feedback. I'm sure you have a better idea of how to properly benchmark differences. I did my best to move forward with this task. I'm more than happy to revisit this approach. The most important here is how much work we can offload from those who need to integrate Gutenberg in WordPress core. I'm not worried about the format at all as long as we can save us from handcrafting the list of script dependencies. See WordPress/wordpress-develop#102 for reference. I also asked a few minutes ago on WordPress Slack in #core-js whether the proposed approach is correct from the perspective of core: https://wordpress.slack.com/archives/C5UNMSU4R/p1569340055065100. It seems like in the case of using JSON or PHP format, we still have the same issue where this coded lands in the folder which should never be exposed to CDN. How about we find a way to put those files in a different folder through another option passed to the Webpack plugin? |
| // Replace `.js` extension with `.asset.php` to find the generated dependencies file. | ||
| $asset_file = substr( $path, 0, -3 ) . '.asset.php'; | ||
| $asset = file_exists( $asset_file ) | ||
| ? require_once( $asset_file ) |
There was a problem hiding this comment.
Is the require_once() important? That is, why not use require()?
With require_once(), if wp_default_scripts runs more than once, the second (any any subsequent) run of each require_once() will just return true, and we'll lose all the dependencies for the scripts.
There was a problem hiding this comment.
In theory, this function doesn't run more than once. However, require makes more sense because the file returns a static value rather than defining a function or class.
Description
This is still work in progress but I'm looking for early feedback.
I'm seeking a way to make assets registration much easier and more automated when you use
wp-scripts build. At the moment we have:https://developer.wordpress.org/reference/functions/wp_register_script/
and
https://developer.wordpress.org/reference/functions/wp_register_style/
They both are very similar and we could further simplify them in some way with a wrapper or by changing its internal implementation:
where WordPress would load the file with
.asset.php(or.asset.json) extension and handle all params based on what Webpack generates.The goal is to make
npm run buildto generate a PHP file[file].asset.phpby default with the object defined as a return statement as described in https://github.com/WordPress/gutenberg/blob/master/docs/rfc/block-registration.md#wordpress-context (handle,dependenciesandversion).I have
dependenciesandversioncovered. JSON is still the default format but this seems like we could offer plain PHP file which can be loaded and cached without any internal processing.JSON example:
{ "dependencies": [ "a", "b" ], "version": "abc" };PHP potential example:
Open questions
json_decodeis fast enough?wp-for core?How has this been tested?
npm testnpm run devnpm run buildTypes of changes
Breaking change for:
@wordpress/dependency-extraction-webpack-plugin@wordpress/scriptsChecklist: