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

Remove empty scripts after webpack compilation #6780

Merged
merged 3 commits into from
Dec 10, 2021

Conversation

delawski
Copy link
Collaborator

@delawski delawski commented Dec 9, 2021

Summary

Fixes #6778

So far, a custom IgnoreExtraneousStyleAssets webpack plugin has been used to prevent generation of empty JS files (e.g. when CSS has been an entry point). After updating webpack from version 4 to 5, the custom plugin stopped working due to webpack internal API changes.

It turns out there is a third-party plugin, webpack-remove-empty-scripts that does exactly what we need. It should be run before DependencyExtractionWebpackPlugin so that empty JS files are removed before .asset.php files are created.

Screenshot 2021-12-09 at 14 08 31

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

So far, a custom `IgnoreExtraneousStyleAssets` webpack plugin has been used to prevent generation of empty JS files (e.g. when CSS has been an entry point). After updating webpack from version 4 to 5, the custom plugin stopped working due to webpack internal API changes.

It turns out there is a third-party plugin, `webpack-remove-empty-scripts` that does exactly what we need. It should be run before `DependencyExtractionWebpackPlugin` so that the empty JS files are removed before empty `.asset.php` files are created.
@delawski delawski added Bug Something isn't working Infrastructure Changes impacting testing infrastructure or build tooling labels Dec 9, 2021
@delawski delawski added this to the v2.2 milestone Dec 9, 2021
@delawski delawski requested a review from westonruter December 9, 2021 13:06
@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2021

Plugin builds for 99a3a89 are ready 🛎️!

@westonruter
Copy link
Member

Here's a comparison before/after:

image

The empty files are all getting deleted as expected, except there's one interesting thing: amp-post-meta-box.js was empty before but now it is not empty. Was this broken before? It seems so! I just checked with the Classic Editor plugin active and there is an error:

image

Clicking the “Edit” link for enabling AMP does nothing. Why was this file empty before?

Checking the same page with the changes in this PR shows shows no errors and the toggle is working:

image

This file was not empty in 2.1.4, so something broke in develop between then and now which you are fixing here.

@westonruter
Copy link
Member

Changes to *.js files between 2.1.4 and this PR:

image

All that looks right.


Changes to *.asset.php files:

image

This looks similarly right, except for the wp-components.asset.php file. This file seems to still be generated incorrectly.

Or maybe not? Is it being generated for wp-components.css and we should actually be updating register_shimmed_styles to use the version in the assets file instead of the hard-coded 13.0.3 (which is probably wrong):

/**
* Registers shimmed assets not guaranteed to be available in core.
*
* @param WP_Styles $wp_styles The WP_Styles instance for the current page.
*/
public function register_shimmed_styles( $wp_styles ) {
$this->override_style(
$wp_styles,
'wp-components',
amp_get_asset_url( 'css/wp-components.css' ),
[],
'13.0.3'
);
}

But if that is the case, then why is the wp-components.asset.php file in the assets/js directory and not the assets/css directory?

These are the changes to the *.css files between 2.1.4 and this PR:

image

@delawski
Copy link
Collaborator Author

This looks similarly right, except for the wp-components.asset.php file. This file seems to still be generated incorrectly.

I think it is the case. This file should not be generated since wp-components.js is bundled with the script files it is used on. The accompanying CSS file is, however, not manually imported to any of the JS entry points meaning we need to rely on Core exposing wp-components.css or polyfill it for older WP versions.

So far, wp-components has been added as one of the entry points in the settingsPage task. This resulted in a superfluous wp-components.asset.php file generation. With 544b66d we add wp-components stylesheet to the styles task, where it better fits. Also, thanks to that, no .asset.php file is created.

Or maybe not? Is it being generated for wp-components.css and we should actually be updating register_shimmed_styles to use the version in the assets file instead of the hard-coded 13.0.3 (which is probably wrong):

I guess we can use the hard-coded value and update it anytime the @wordpress/components package is bumped (alternatively use the plugin version instead).

@westonruter
Copy link
Member

I guess we can use the hard-coded value and update it anytime the @wordpress/components package is bumped (alternatively use the plugin version instead).

Good idea. I've done that in 99a3a89.

@westonruter
Copy link
Member

So far, wp-components has been added as one of the entry points in the settingsPage task. This resulted in a superfluous wp-components.asset.php file generation. With 544b66d we add wp-components stylesheet to the styles task, where it better fits. Also, thanks to that, no .asset.php file is created.

Perfect!

@westonruter westonruter disabled auto-merge December 10, 2021 17:12
@westonruter westonruter merged commit 7e67d49 into develop Dec 10, 2021
@westonruter westonruter deleted the fix/prevent-empty-js-compilation branch December 10, 2021 17:12
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. Infrastructure Changes impacting testing infrastructure or build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty JS files are generated as part of build
2 participants