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

Add Webpack plugin to ignore JS and PHP asset files when stylesheet used as entrypoint #6078

Merged
merged 7 commits into from
Apr 21, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
58 changes: 58 additions & 0 deletions bin/ignore-extraneous-style-assets.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/**
* Webpack plugin to prevent extraneous assets from being emitted for stylesheets.
*
* JS files and PHP asset files generated by `DependencyExtractionWebpackPlugin` are currently ignored.
*/
class IgnoreExtraneousStyleAssets {
isCssFile( file ) {
return /^(?!!!).+\.(?:sc|sa|c)ss$/.test( file );
}

generateIgnoreRegex( entries ) {
const expression = entries
.reduce( ( acc, entry ) => `${ acc }|${ entry }.js|${ entry }.asset.php`, '' )
.substr( 1 );
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 thinking this could be more clear:

Suggested change
.substr( 1 );
.trimStart( '|' );

But alas trimStart() doesn't take any arguments like ltrim() does in PHP.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also it seems that substr() is depracated and substring() should be used instead.

So ideally this would be something like:

Suggested change
.substr( 1 );
.substring( 1 ); // Remove a leading '|'

@westonruter to your point - I guess we can use a comment to clarify.

pierlon marked this conversation as resolved.
Show resolved Hide resolved

return new RegExp( expression );
}

apply( compiler ) {
const cssEntries = [];

compiler.hooks.compilation.tap( 'IgnoreExtraneousStyleAssets', ( compilation ) => {
// The `addEntry` compilation hook is undocumented. As for why, ¯\_(ツ)_/¯.
compilation.hooks.addEntry.tap( 'IgnoreExtraneousStyleAssets', ( entry, name ) => {
if ( entry.type === 'single entry' ) {
if ( this.isCssFile( entry.request ) ) {
cssEntries.push( name );
}
} else if ( entry.type === 'multi entry' ) {
const filteredDependencies = entry.dependencies.filter(
( singleDependency ) => this.isCssFile( singleDependency.request ),
);

// If all dependencies of the entry are CSS files, add it to the list of entries to ignore.
if ( entry.dependencies.length === filteredDependencies.length ) {
cssEntries.push( name );
}
}
} );
} );

compiler.hooks.emit.tap( 'IgnoreExtraneousStyleAssets', ( compilation ) => {
if ( cssEntries.length === 0 ) {
return;
}

const ignoreRegex = this.generateIgnoreRegex( cssEntries );

Object.keys( compilation.assets ).forEach( ( assetName ) => {
if ( ignoreRegex.test( assetName ) ) {
delete compilation.assets[ assetName ];
}
} );
} );
}
}

module.exports = IgnoreExtraneousStyleAssets;
185 changes: 0 additions & 185 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@
"grunt-contrib-copy": "1.0.0",
"grunt-shell": "3.0.1",
"grunt-wp-deploy": "2.1.2",
"ignore-emit-webpack-plugin": "2.0.6",
"jest-silent-reporter": "0.5.0",
"lint-staged": "10.5.4",
"lodash": "4.17.21",
Expand All @@ -92,7 +91,6 @@
"react-test-renderer": "16.14.0",
"rtlcss-webpack-plugin": "4.0.6",
"svgo": "2.3.0",
"terser-webpack-plugin": "4.2.3",
"webpackbar": "4.0.0"
},
"scripts": {
Expand Down
9 changes: 6 additions & 3 deletions tests/php/bootstrap.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,16 @@ function amp_unit_test_load_plugin_file() {

tests_add_filter( 'muplugins_loaded', 'amp_unit_test_load_plugin_file' );

// Start up the WP testing environment.
require $_test_root . '/includes/bootstrap.php';

/*
* Load WP CLI. Its test bootstrap file can't be required as it will load
* duplicate class names which are already in use.
*/
define( 'WP_CLI_ROOT', TESTS_PLUGIN_DIR . '/vendor/wp-cli/wp-cli' );
define( 'WP_CLI_VENDOR_DIR', TESTS_PLUGIN_DIR . '/vendor' );
require_once WP_CLI_ROOT . '/php/utils.php';

$logger = new WP_CLI\Loggers\Regular( true );
WP_CLI::set_logger( $logger );
Comment on lines +53 to +54
Copy link
Contributor Author

Choose a reason for hiding this comment

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

PHPUnit tests were failing due to a call to WP_CLI::warning() failing to log a message:

amp-wp/amp.php

Line 217 in 1e7ea02

WP_CLI::warning( $message );

PHP Fatal error: Uncaught Error: Call to a member function warning() on null in /tmp/wordpress/src/wp-content/plugins/amp/vendor/wp-cli/wp-cli/php/class-wp-cli.php:808

It turns out the deafult WP CLI logger is set defined when WP_CLI::bootstrap() is called, but that isn't applicable in a testing environment. Instead, I've manually initialized and set a logger during the PHPUnit bootstrap process.

Copy link
Member

Choose a reason for hiding this comment

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

What is the warning that is being emitted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The amp-block-editor.js file was previously not being emitted by Webpack (which is now fixed in 4b47698). Because of that the following error was being emitted:

amp-wp/amp.php

Lines 160 to 169 in 1e7ea02

if ( ! file_exists( AMP__DIR__ . '/vendor/autoload.php' ) || ! file_exists( AMP__DIR__ . '/vendor/sabberworm/php-css-parser' ) || ! file_exists( AMP__DIR__ . '/assets/js/amp-block-editor.js' ) ) {
$_amp_load_errors->add(
'build_required',
sprintf(
/* translators: %s: composer install && npm install && npm run build:prod */
__( 'You appear to be running the AMP plugin from source. Please do %s to finish installation.', 'amp' ), // phpcs:ignore WordPress.Security.EscapeOutput
'<code>composer install &amp;&amp; npm install &amp;&amp; npm run build:prod</code>'
)
);
}

Copy link
Member

Choose a reason for hiding this comment

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

So now the warning is not being emitted? But if it were to be emitted again, then the logger change will ensure the warning will be emitted without a failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and yes.


// Start up the WP testing environment.
require $_test_root . '/includes/bootstrap.php';
Loading