-
Notifications
You must be signed in to change notification settings - Fork 381
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
Add Webpack plugin to ignore JS and PHP asset files when stylesheet used as entrypoint #6078
Conversation
…sed as entrypoint
@@ -334,6 +335,8 @@ const settingsPage = { | |||
} | |||
}, | |||
} ), | |||
// Ensure assets generated by `DependencyExtractionWebpackPlugin` are also ignored for styles entries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should prevent the empty JS chunk and PHP asset file for the wp-components
entry from being generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plugin builds for 6c9e5da are ready 🛎️!
|
Investigating CI issues... |
Codecov Report
@@ Coverage Diff @@
## develop #6078 +/- ##
=============================================
- Coverage 74.92% 74.91% -0.02%
Complexity 5775 5775
=============================================
Files 231 231
Lines 17484 17487 +3
=============================================
Hits 13100 13100
- Misses 4384 4387 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
$logger = new WP_CLI\Loggers\Regular( true ); | ||
WP_CLI::set_logger( $logger ); |
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
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 && npm install && npm run build:prod</code>' | |
) | |
); | |
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Output checks out!
I defer to @delawski for the webpack changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new TerserPlugin( { | ||
parallel: true, | ||
sourceMap: false, | ||
cache: true, | ||
terserOptions: { | ||
output: { | ||
comments: /translators:/i, | ||
}, | ||
}, | ||
extractComments: false, | ||
} ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity: why are we no longer overriding the TerserPlugin
config (or why have we done so before at all)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The config we're importing from @wordpress/scripts
already configures the TerserPlugin
the same way we do (with the additional config of not mangling translations).
As for why we were overriding it, we actually were not. The TerserPlugin
config was only "recently" added to the default Webpack config (see WordPress/gutenberg#22990) (relative to when this plugin config was first added, that is).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation 🙏
webpack.config.js
Outdated
entry: () => { | ||
const entries = {}; | ||
const dir = './assets/css/src'; | ||
fs.readdirSync( dir ).forEach( ( fileName ) => { | ||
entries[ fileName.replace( /\.[^/.]+$/, '' ) ] = `${ dir }/${ fileName }`; | ||
} ); | ||
|
||
return entries; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that!
…lugin # Conflicts: # webpack.config.js
Co-authored-by: Piotr Delawski <[email protected]>
Thanks for the review @delawski and @westonruter. This should be good to ship now 😄. |
.flat() | ||
.join( '|' ); | ||
.reduce( ( acc, entry ) => `${ acc }|${ entry }.js|${ entry }.asset.php`, '' ) | ||
.substr( 1 ); |
There was a problem hiding this comment.
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:
.substr( 1 ); | |
.trimStart( '|' ); |
But alas trimStart()
doesn't take any arguments like ltrim()
does in PHP.
There was a problem hiding this comment.
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:
.substr( 1 ); | |
.substring( 1 ); // Remove a leading '|' |
@westonruter to your point - I guess we can use a comment to clarify.
@pierlon There's a build error now:
|
Co-authored-by: Piotr Delawski <[email protected]>
@@ -349,7 +349,10 @@ const styles = { | |||
const entries = {}; | |||
const dir = './assets/css/src'; | |||
fs.readdirSync( dir ).forEach( ( fileName ) => { | |||
entries[ fileName.replace( /\.[^/.]+$/, '' ) ] = `${ dir }/${ fileName }`; | |||
const fullPath = `${ dir }/${ fileName }`; | |||
if ( ! fs.lstatSync( fullPath ).isDirectory() ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this to ignore directories in assets/css/src
, i.e. only root files in that directory will be used as entries.
QA passed. Found no issues in the "Network" tab in the Chrome dev tools. |
Summary
Follow up of #6057.
This PR adds a custom Webpack plugin that identifies stylesheet entries and prevents empty JS chunks and PHP asset files from being emitted for each entry. The plugin has been added to the shared config so that each Webpack config now makes use of it by default.
Checklist