Dependency Extraction: output asset.php files for shared chunks too #41002
Conversation
jsnajdr
added
the
[Package] Dependency Extraction Webpack Plugin
|
After this PR, we have asset files for all JS assets, but we still don't know what we need to load the Or can we co-opt the existing Also, we're not handling CSS assets. Gutenberg packages usually have just one |
|
Size Change: 0 B Total Size: 1.25 MB ℹ️ View Unchanged
|
jsnajdr
force-pushed
the
update/dep-extr-asset-files-for-shared-chunks
branch
from
e977968
to
55fb9fb
Compare
|
Thank you so much for attacking this complex problem ❤️ I’ll try to take a deeper dive into proposed implementation later this week. |
|
One idea how to avoid calculating the instead of just When writing files to disk, webpack will remove the fyi @anomiex |
|
Keep in mind that this package is released for third parties to use too, who may not share your constraints.
In Jetpack we're already using mini-css-extract-plugin to extract CSS from our bundles. We're currently using the version from
If you go with that, your code should gracefully fail if someone tries to use the plugin without having the configuration set in that manner. |
Well this means that we don't really want the But here we calculate, and want to calculate, the combined hash of all the chunk's assets, both .js and .css. If we used just the The relevant assets are Thanks for clarifying the requirements. |
I can confirm it works as expected.
I noticed that we cover every possible case for JavaScript files but dynamic imports. It still works correctly as all dependencies get collected in the entry point's asset file, but sometimes it might be suboptimal. It's an edge case and I expect it would be hard to handle correctly anyway so we can live with this limitation. Regarding the information about the existing shared dependencies created by webpack, it's also the tricky part that we can address separately. One idea I can think of, would be referencing the path to the asset file next to all other usual dependencies.
I'm noting that with the changes proposed in this PR the |
Chunks that are dynamically imported don't need to be externally visible -- their file names are webpack's internal implementation detail. It's the webpack runtime that knows how to load them. The external world only needs to know how to load an entrypoint. Then the webpack runtime takes over and handles everything else. In that sense, we only need to generate manifests for entrypoints.
That's true, and it's a blocker that prevents merging this PR as is. It would break Jetpack. |
It was quite some time when I was working with dynamic imports. In the block editor, we don't use lazy loading so I wasn't sure about that part. Thank you for clarifying that.
Let's address that point and land the rest of changes 💯 |
jsnajdr
force-pushed
the
update/dep-extr-asset-files-for-shared-chunks
branch
2 times, most recently
from
82f6103
to
316b936
Compare
I changed the content hash calculation algorithm to include all assets in the chunk, both CSS and JS. As the previous code did. As the unit tests show, all the hashes remained the same as they were before this PR. The only change is that we're outputting a new I believe we're ready to 🚢 here. |
|
[deleted: never mind, I didn't have the latest copy checked out thanks to the force-push] |
|
|
||
| const chunkJSFile = chunkFiles.find( ( f ) => /\.js$/i.test( f ) ); | ||
| if ( ! chunkJSFile ) { | ||
| // There's no JS file in this chunk, no work for us. Typically a `style.css` from cache group. |
There was a problem hiding this comment.
Does this hold true for third parties who might be using this and processing CSS in different ways? Or might they still want the asset file?
There was a problem hiding this comment.
If that is the case then it would be accidental. I think it's fine to proceed as is and revisit this approach if we hear back about the exact use cases projects could have.
There was a problem hiding this comment.
I was using the generated asset.php files for standalone CSS files output from Webpack to provide a version number, e.g.:
$componentAndHookStylesMetadata =
require MY_DIR . '/build/component-and-hook-styles.asset.php';
wp_enqueue_style(
'my-plugin-component-and-hook-styles',
plugin_dir_url(__DIR__) . 'build/component-and-hook-styles.css',
[],
$componentAndHookStylesMetadata['version']
);It seems the changes in this PR have broken this, resulting in fatal errors when my plugin tries to load the asset.php file that is no longer created.
I've worked around this by changing the code to:
$lastModifiedTimestamp =
filemtime(plugin_dir_path(__DIR__) . 'build/component-and-hook-styles.css');
// (DT::now() is just a custom util to get a DateTimeImmutable for the current moment.)
if ($lastModifiedTimestamp === false) $lastModifiedTimestamp = DT::now()->getTimestamp();
wp_enqueue_style(
'my-plugin-component-and-hook-styles',
plugin_dir_url(__DIR__) . 'build/component-and-hook-styles.css',
[],
(string) $lastModifiedTimestamp,
);Maybe this is how I should've been handling it from the start, but I figured I mention that this did indeed affect at least one person.
There was a problem hiding this comment.
@ZebulanStanphill, how would you get the asset file generated for the CSS file? Was it set explicitly as an entry point?
It's also possible to use the version field in the block.json file, so it gets automatically used with CSS files:
https://github.com/WordPress/gutenberg/blob/trunk/docs/reference-guides/block-api/block-metadata.md#version
The solution you have works great but only in case when your block is always loaded from the same server.
There was a problem hiding this comment.
@gziolo Yeah, I had an SCSS file as one of my Webpack entry points, and I used mini-css-extract-plugin to put the styles in a CSS file, and webpack-remove-empty-scripts to remove the empty .js file generated from the entry point.
Thanks for the advice on using a version field on the block.json. The project I'm working on is internal and only used on a single site and server anyway, though, so I suppose it's fine to continue using filemtime in my case. (And also, I'd probably forget to update the version field on the blocks if I was using that, haha.)
There was a problem hiding this comment.
I would expect that updating version is easy to miss, so maybe we should figure out how to generate asset files also for all CSS files after all. It definitely didn't produce asset files for CSS files extracted when importing them from JS. We never explored that because in CSS, unlike in JavaScript, we don't need to sort out their dependent styles.
gziolo
force-pushed
the
update/dep-extr-asset-files-for-shared-chunks
branch
from
fa78e1c
to
3a44214
Compare
There was a problem hiding this comment.
I rebase this PR with the latest changes from trunk. I also included an entry in the CHANGELOG file.
Everything tests well. We should look into better integration between the newly generated asset files for shared chunks with the regular chunks as the next step.
When there is a webpack project where an entrypoint has multiple chunks, then assumptions that the deps extraction plugin kind of break down.
The plugin assumes that when there is an entrypoint called
main, it has exactly one JS file calledmain.jsand that's all we need to load. Amain.asset.phpfile is produced, and we can use it to enqueue a WordPress script:But an entrypoint can have multiple chunks, for example:
entry-main.jsis the primary script file formainentrypoint, and can have arbitrary nameruntime.jsis a shared webpack runtime created with theruntime: 'single'optionshared.jsis a chunk shared with another entrypoint in the same project, with common codevendor-lib.jsis a separate chunk with stable library code, created using theoptimization.splitChunks.cacheGroupsoptionCurrently there is no
asset.phpinfo indicating that these chunks exist and need to be loaded. There will bemain.asset.phpfile that doesn't even point correctly to theentry-main.jsfile.This PR is a step towards solving this. For the example above, it will produce four asset files,
entry-main.asset.php,runtime.asset.php,shared.asset.phpandvendor-lib.asset.php. Each of them will specify dependencies used by that particular chunk, and a content hash of that one specific.jsfile in theversionfield.For the common case it doesn't change anything. You can see that from the unit test snapshot updates, where the only modification is for the
runtime-singlecase, where there is a runtime chunk that newly gets aruntime.asset.phpfile.The PR rewrites the loop that traverses the entrypoints and chunks from:
to
For the
runtime-singleexample, the structure of entrypoints and chunks would be like:In the old code, each content hash would be calculated from three files:
a.js,a.cssandruntime.js, and filesa.asset.phpandb.asset.phpwould be written.In the new code, we first construct a set of entrypoint chunks. There are three, one of them shared. And for each of these chunks we'll compute hash for just the
.jsfile, and write filesa.asset.php,b.asset.php, andruntime.asset.php.