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 unminified source JS files to builds #1643

Merged

Conversation

ShyamGadde
Copy link
Contributor

@ShyamGadde ShyamGadde commented Nov 11, 2024

Summary

Fixes #1493

Relevant technical choices

  • The unminified version of web-vitals.js is not included because the file is copied directly from the pre-minified version in node_modules.
  • Source maps are excluded, as the unminified development version of the script will be loaded when SCRIPT_DEBUG is enabled.

@ShyamGadde ShyamGadde marked this pull request as ready for review November 11, 2024 17:05
Copy link

github-actions bot commented Nov 11, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ShyamGadde <shyamgadde@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@westonruter westonruter added [Plugin] Optimization Detective Issues for the Optimization Detective plugin [Type] Enhancement A suggestion for improvement of an existing feature labels Nov 11, 2024
Comment on lines 79 to 82
$detection_script_file = 'detect.min.js';
if ( defined( 'SCRIPT_DEBUG' ) && SCRIPT_DEBUG ) {
$detection_script_file = 'detect.js';
}
Copy link
Member

Choose a reason for hiding this comment

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

We should be able to reuse wp_scripts_get_suffix() here as seen in wp_default_scripts() in core:

Suggested change
$detection_script_file = 'detect.min.js';
if ( defined( 'SCRIPT_DEBUG' ) && SCRIPT_DEBUG ) {
$detection_script_file = 'detect.js';
}
$detection_script_file = sprintf( 'detect%s.js', wp_scripts_get_suffix() );

@westonruter
Copy link
Member

  • The unminified version of web-vitals.js is not included because the file is copied directly from the pre-minified version in node_modules.

I was thinking alternatively that we could load the web-vitals file from src instead of dist when SCRIPT_DEBUG is enabled and there is a local node_modules directory. But this isn't feasible either because the source files are in TypeScript, so this would require transpilation. Probably doesn't make sense.

@westonruter
Copy link
Member

westonruter commented Nov 11, 2024

Oh, there are also now 3 other JS files that need to have their minified version included conditionally:

  1. plugins/embed-optimizer/detect.js
  2. plugins/embed-optimizer/lazy-load.js
  3. plugins/image-prioritizer/lazy-load.js

@@ -164,6 +168,7 @@ const buildPlugin = ( env ) => {
{
from,
to,
info: { minimized: true },
Copy link
Member

Choose a reason for hiding this comment

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

This won't attempt to create a minified version of web-vitals.js, will it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, actually it's the opposite - setting info: { minimized: true } tells Webpack to skip running these files through the minimizer. It's a way to tell Webpack to copy the files 'as is' without trying to minimize them. The documentation specifically mentions this is 'Useful if you need to simply copy *.js files to destination "as is" without evaluating and minimizing them using Terser.'

ref: https://webpack.js.org/plugins/copy-webpack-plugin/#skip-running-javascript-files-through-a-minimizer

Copy link
Member

Choose a reason for hiding this comment

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

OK, thank you. I'm a little confused. We want to create minified versions of all the scripts, except for those which are copied from node_modules, right? So we'd want to copy from node_modules/web-vitals/dist and node_modules/@builder.io/partytown as pre-minified, but everything else we'd want to create minified versions for. I don't see how that is all configured here. (I'm a webpack novice!)

Copy link
Contributor Author

@ShyamGadde ShyamGadde Nov 12, 2024

Choose a reason for hiding this comment

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

Let me clarify the current state and what needs to be fixed:

  1. For files from node_modules (web-vitals and partytown):

    • These come pre-minified, but when copying them to the build directory for their respective plugins, we're not telling Webpack that. Currently, I have configured minimization skipping only for the production build.
    • Need to add info: { minimized: true } to prevent re-minification attempts
  2. For plugin JS files that need minified versions:

    • Minified version for optimization-detective/detect.js is being created (detect.min.js)
    • Need to add minified versions for (I missed these as they were not initially part of the issue description):
      • embed-optimizer/detect.js
      • embed-optimizer/lazy-load.js
      • image-prioritizer/lazy-load.js

I'll update this PR to handle both these cases. Would that address your concerns?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I believe so!

@@ -61,16 +61,20 @@ const optimizationDetective = ( env ) => {
patterns: [
{
from: `${ source }/dist/web-vitals.js`,
to: `${ destination }/web-vitals.js`,
to: `${ destination }/build/web-vitals.js`,
Copy link
Member

Choose a reason for hiding this comment

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

Is this missing the flag to say it is minified?

Suggested change
to: `${ destination }/build/web-vitals.js`,
to: `${ destination }/build/web-vitals.js`,
info: { minimized: true }`,

Copy link
Contributor Author

@ShyamGadde ShyamGadde Nov 12, 2024

Choose a reason for hiding this comment

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

Yes! I just realized this. I should probably make this change in the config for Web Worker Offloading as well, which is copying the partytown files.

@@ -164,6 +168,7 @@ const buildPlugin = ( env ) => {
{
from,
to,
info: { minimized: true },
Copy link
Member

Choose a reason for hiding this comment

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

OK, thank you. I'm a little confused. We want to create minified versions of all the scripts, except for those which are copied from node_modules, right? So we'd want to copy from node_modules/web-vitals/dist and node_modules/@builder.io/partytown as pre-minified, but everything else we'd want to create minified versions for. I don't see how that is all configured here. (I'm a webpack novice!)

@ShyamGadde
Copy link
Contributor Author

I wanted to bring up something I noticed regarding the partytown package used in the web worker offloading plugin:

  • The partytown package does provide unminified files under node_modules/@builder.io/partytown/lib/debug
  • In the initial webpack config, these were being copied but also getting minified
  • Now that we're skipping/disabling minification, we could potentially use these unminified script files when SCRIPT_DEBUG is enabled

Since the partytown code is inlined as a script tag, should I modify this plugin to use the unminified partytown JavaScript when SCRIPT_DEBUG is enabled? Or would it be better to stick with the current approach of using the pre-minified files?

@westonruter
Copy link
Member

@ShyamGadde definitely! The non-minified debug version should be used here when SCRIPT_DEBUG is enabled:

$partytown_js = file_get_contents( __DIR__ . '/build/partytown.js' ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents -- It's a local filesystem path not a remote request.

@westonruter
Copy link
Member

And good catch on the config erroneously minifying the debug scripts:

https://plugins.trac.wordpress.org/browser/web-worker-offloading/tags/0.1.1/build/partytown-atomics.js
https://plugins.trac.wordpress.org/browser/web-worker-offloading/tags/0.1.1/build/debug/partytown-atomics.js

The second one shouldn't have been minified!

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

Great work! It's everything I ever dreamed for!

@westonruter westonruter added [Plugin] Embed Optimizer Issues for the Embed Optimizer plugin (formerly Auto Sizes) [Plugin] Image Prioritizer Issues for the Image Prioritizer plugin (dependent on Optimization Detective) [Plugin] Web Worker Offloading Issues for the Web Worker Offloading plugin. labels Nov 13, 2024
@westonruter westonruter merged commit df29967 into WordPress:trunk Nov 13, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Embed Optimizer Issues for the Embed Optimizer plugin (formerly Auto Sizes) [Plugin] Image Prioritizer Issues for the Image Prioritizer plugin (dependent on Optimization Detective) [Plugin] Optimization Detective Issues for the Optimization Detective plugin [Plugin] Web Worker Offloading Issues for the Web Worker Offloading plugin. [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unminified source JS files and source maps are not distributed with builds
2 participants