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.

Sign up for GitHub

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

Dependency Extraction: output asset.php files for shared chunks too #41002

Merged
merged 3 commits into from
Jul 1, 2022
Merged

Dependency Extraction: output asset.php files for shared chunks too #41002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e06c583
Add new test case for project with CSS modules
jsnajdr May 11, 2022
3a44214
Dependency Extraction: output asset.php files for shared chunks too
jsnajdr May 11, 2022
cf2c601
Add CHANGELOG entry
gziolo Jul 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/dependency-extraction-webpack-plugin/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### New Features

- Output asset files for shared chunks, too ([#41002](https://github.com/WordPress/gutenberg/pull/41002)).

## 3.5.0 (2022-05-18)

### Bug Fix
Expand Down
143 changes: 57 additions & 86 deletions packages/dependency-extraction-webpack-plugin/lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ class DependencyExtractionWebpackPlugin {

if ( isWebpack4 ) {
compiler.hooks.emit.tap( this.constructor.name, ( compilation ) =>
this.addAssets( compilation, compiler )
this.addAssets( compilation )
);
} else {
compiler.hooks.thisCompilation.tap(
Expand All @@ -129,14 +129,14 @@ class DependencyExtractionWebpackPlugin {
stage: compiler.webpack.Compilation
.PROCESS_ASSETS_STAGE_ANALYSE,
},
() => this.addAssets( compilation, compiler )
() => this.addAssets( compilation )
);
}
);
}
}

addAssets( compilation, compiler ) {
addAssets( compilation ) {
const {
combineAssets,
combinedOutputFile,
Expand All @@ -162,124 +162,102 @@ class DependencyExtractionWebpackPlugin {

const combinedAssetsData = {};

// Process each entry point independently.
for ( const [
entrypointName,
entrypoint,
] of compilation.entrypoints.entries() ) {
const entrypointExternalizedWpDeps = new Set();
// Accumulate all entrypoint chunks, some of them shared
const entrypointChunks = new Set();
for ( const entrypoint of compilation.entrypoints.values() ) {
for ( const chunk of entrypoint.chunks ) {
entrypointChunks.add( chunk );
}
}

// Process each entrypoint chunk independently
for ( const chunk of entrypointChunks ) {
const chunkFiles = Array.from( chunk.files );

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

continue;
}

const chunkDeps = new Set();
if ( injectPolyfill ) {
entrypointExternalizedWpDeps.add( 'wp-polyfill' );
chunkDeps.add( 'wp-polyfill' );
}

const processModule = ( { userRequest } ) => {
if ( this.externalizedDeps.has( userRequest ) ) {
const scriptDependency =
this.mapRequestToDependency( userRequest );
entrypointExternalizedWpDeps.add( scriptDependency );
chunkDeps.add( this.mapRequestToDependency( userRequest ) );
}
};

// Search for externalized modules in all chunks.
for ( const chunk of entrypoint.chunks ) {
const modulesIterable = isWebpack4
? chunk.modulesIterable
: compilation.chunkGraph.getChunkModules( chunk );
for ( const chunkModule of modulesIterable ) {
processModule( chunkModule );
// Loop through submodules of ConcatenatedModule.
if ( chunkModule.modules ) {
for ( const concatModule of chunkModule.modules ) {
processModule( concatModule );
}
const modulesIterable = isWebpack4
? chunk.modulesIterable
: compilation.chunkGraph.getChunkModules( chunk );
for ( const chunkModule of modulesIterable ) {
processModule( chunkModule );
// Loop through submodules of ConcatenatedModule.
if ( chunkModule.modules ) {
for ( const concatModule of chunkModule.modules ) {
processModule( concatModule );
}
}
}

const { hashFunction, hashDigest, hashDigestLength } =
compilation.outputOptions;

// Go through the assets and hash the sources. We can't just use
// `entrypointChunk.contentHash` because that's not updated when
// `chunk.contentHash` because that's not updated when
// assets are minified. In practice the hash is updated by
// `RealContentHashPlugin` after minification, but it only modifies
// already-produced asset filenames and the updated hash is not
// available to plugins.
const hash = createHash( hashFunction );
for ( const filename of entrypoint.getFiles().sort() ) {
const asset = compilation.getAsset( filename );
hash.update( asset.source.buffer() );
}
const version = hash
const { hashFunction, hashDigest, hashDigestLength } =
compilation.outputOptions;

const contentHash = chunkFiles
gziolo marked this conversation as resolved.
Show resolved Hide resolved
gziolo marked this conversation as resolved.
Show resolved Hide resolved
.sort()
.reduce( ( hash, filename ) => {
const asset = compilation.getAsset( filename );
return hash.update( asset.source.buffer() );
}, createHash( hashFunction ) )
gziolo marked this conversation as resolved.
Show resolved Hide resolved
.digest( hashDigest )
.slice( 0, hashDigestLength );

const entrypointChunk = isWebpack4
? entrypoint.chunks.find( ( c ) => c.name === entrypointName )
: entrypoint.getEntrypointChunk();

const assetData = {
// Get a sorted array so we can produce a stable, stringified representation.
dependencies: Array.from( entrypointExternalizedWpDeps ).sort(),
version,
dependencies: Array.from( chunkDeps ).sort(),
version: contentHash,
};

const assetString = this.stringify( assetData );
const contentHash = createHash( hashFunction )
.update( assetString )
.digest( hashDigest )
.slice( 0, hashDigestLength );

// Determine a filename for the asset file.
const [ filename, query ] = entrypointName.split( '?', 2 );
const buildFilename = compilation.getPath(
compiler.options.output.filename,
{
chunk: entrypointChunk,
filename,
query,
basename: basename( filename ),
contentHash,
}
);

if ( combineAssets ) {
combinedAssetsData[ buildFilename ] = assetData;
combinedAssetsData[ chunkJSFile ] = assetData;
continue;
}

let assetFilename;

if ( outputFilename ) {
assetFilename = compilation.getPath( outputFilename, {
chunk: entrypointChunk,
filename,
query,
basename: basename( filename ),
chunk,
filename: chunkJSFile,
contentHash,
} );
} else {
assetFilename = buildFilename.replace(
/\.js$/i,
'.asset.' + ( outputFormat === 'php' ? 'php' : 'json' )
);
const suffix =
'.asset.' + ( outputFormat === 'php' ? 'php' : 'json' );
assetFilename = compilation
.getPath( '[file]', { filename: chunkJSFile } )
.replace( /\.js$/i, suffix );
}

// Add source and file into compilation for webpack to output.
compilation.assets[ assetFilename ] = new RawSource( assetString );
entrypointChunk.files[ isWebpack4 ? 'push' : 'add' ](
assetFilename
compilation.assets[ assetFilename ] = new RawSource(
this.stringify( assetData )
);
chunk.files[ isWebpack4 ? 'push' : 'add' ]( assetFilename );
}

if ( combineAssets ) {
// Assert the `string` type for output path.
// The type indicates the option may be `undefined`.
// However, at this point in compilation, webpack has filled the options in if
// they were not provided.
const outputFolder = /** @type {{path:string}} */ (
compiler.options.output
).path;
const outputFolder = compilation.outputOptions.path;

const assetsFilePath = path.resolve(
outputFolder,
Expand All @@ -299,11 +277,4 @@ class DependencyExtractionWebpackPlugin {
}
}

function basename( name ) {
if ( ! name.includes( '/' ) ) {
return name;
}
return name.substr( name.lastIndexOf( '/' ) + 1 );
}

module.exports = DependencyExtractionWebpackPlugin;
32 changes: 30 additions & 2 deletions packages/dependency-extraction-webpack-plugin/test/__snapshots__/build.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,12 +207,17 @@ Array [
`;

exports[`DependencyExtractionWebpackPlugin Webpack \`runtime-chunk-single\` should produce expected output: Asset file 'a.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array('wp-blob'), 'version' => '39c05211520759f42c9d');
"<?php return array('dependencies' => array('wp-blob'), 'version' => '09a0c551770a351c5ca7');
"
`;

exports[`DependencyExtractionWebpackPlugin Webpack \`runtime-chunk-single\` should produce expected output: Asset file 'b.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array('lodash', 'wp-blob'), 'version' => '70fbf918dd6a71b65cf6');
"<?php return array('dependencies' => array('lodash', 'wp-blob'), 'version' => 'c9f00d690a9f72438910');
"
`;

exports[`DependencyExtractionWebpackPlugin Webpack \`runtime-chunk-single\` should produce expected output: Asset file 'runtime.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array(), 'version' => '46ea0ff11ac53fa5e88b');
"
`;

Expand All @@ -234,6 +239,29 @@ Array [
]
`;

exports[`DependencyExtractionWebpackPlugin Webpack \`style-imports\` should produce expected output: Asset file 'main.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array('lodash', 'wp-blob'), 'version' => 'd8c0ee89d933a3809c0e');
"
`;

exports[`DependencyExtractionWebpackPlugin Webpack \`style-imports\` should produce expected output: External modules should match snapshot 1`] = `
Array [
Object {
"externalType": "window",
"request": "lodash",
"userRequest": "lodash",
},
Object {
"externalType": "window",
"request": Array [
"wp",
"blob",
],
"userRequest": "@wordpress/blob",
},
]
`;

exports[`DependencyExtractionWebpackPlugin Webpack \`wordpress\` should produce expected output: Asset file 'main.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array('lodash', 'wp-blob'), 'version' => '9b7ebe61044661fdabda');
"
Expand Down
13 changes: 2 additions & 11 deletions packages/dependency-extraction-webpack-plugin/test/build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,17 +52,8 @@ describe( 'DependencyExtractionWebpackPlugin', () => {
const assetFiles = glob(
`${ outputDirectory }/+(*.asset|assets).@(json|php)`
);
const hasCombinedAssets = ( options.plugins || [] ).some(
( plugin ) => !! ( plugin.options || {} ).combineAssets
);
const entrypointCount =
typeof options.entry === 'object'
? Object.keys( options.entry ).length
: 1;
const expectedLength = hasCombinedAssets
? 1
: entrypointCount;
expect( assetFiles ).toHaveLength( expectedLength );

expect( assetFiles.length ).toBeGreaterThan( 0 );

// Asset files should match.
assetFiles.forEach( ( assetFile ) => {
Expand Down
16 changes: 16 additions & 0 deletions packages/dependency-extraction-webpack-plugin/test/fixtures/style-imports/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* WordPress dependencies
*/
import { isBlobURL } from '@wordpress/blob';

/**
* External dependencies
*/
import _ from 'lodash';

/**
* Internal dependencies
*/
import './style.css';

_.isEmpty( isBlobURL( '' ) );
1 change: 1 addition & 0 deletions packages/dependency-extraction-webpack-plugin/test/fixtures/style-imports/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
body { color: white; }
31 changes: 31 additions & 0 deletions packages/dependency-extraction-webpack-plugin/test/fixtures/style-imports/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/**
* External dependencies
*/
const MiniCSSExtractPlugin = require( 'mini-css-extract-plugin' );

/**
* Internal dependencies
*/
const DependencyExtractionWebpackPlugin = require( '../../..' );

module.exports = {
plugins: [
new DependencyExtractionWebpackPlugin(),
new MiniCSSExtractPlugin(),
],
module: {
rules: [
{
test: /\.css$/,
use: [
{
loader: MiniCSSExtractPlugin.loader,
},
{
loader: require.resolve( 'css-loader' ),
},
],
},
],
},
};