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

Don't include static files in the build. #43

Merged
merged 7 commits into from
Oct 14, 2022

Conversation

dd32
Copy link
Member

@dd32 dd32 commented Oct 14, 2022

Fixes #39, See #2

This appears to reduce the bundle size significantly, unless I've done something massively wrong.

# With this PR
$ du -h ./dist-web/wp.data
 13M	./dist-web/wp.data

# Resetting to committed file
$ git checkout ./dist-web/wp.data
$ du -h ./dist-web/wp.data
 44M	./dist-web/wp.data

Testing

  1. Apply PR
  2. npm run build:wp
  3. npm run dev

Comment on lines +66 to +76
sed "s/<?php/<?php define( 'CONCATENATE_SCRIPTS', false );/" wp-config.php > wp-config.php.new
mv wp-config.php.new wp-config.php
Copy link
Collaborator

@adamziel adamziel Oct 14, 2022

Choose a reason for hiding this comment

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

I'd move this to src/shared/wordpress.mjs just because all the other WordPress customizations live there – I'd like to avoid the fragmentation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, on the other hand this is a build feature. You can't remove assets without setting CONCATENATE_SCRIPTS to false. Alright – good call putting it here!

@adamziel
Copy link
Collaborator

adamziel commented Oct 14, 2022

YAY! I was hoping this is possible somehow! Thank you @dd32, 31 MB is a huge win!

About the change – wp-admin seems to work great but I noticed some missing styles on the frontend:

CleanShot 2022-10-14 at 12 35 54@2x

It doesn't seem to request /wp-includes/blocks/navigation/view.min.js for some reason. Maybe it checks whether the file_exists() somewhere?

Edit: WordPress does indeed check that in multiple places like register_block_style_handle, register_block_script_handle, or block_type_metadata_view_script. Maybe we could truncate the files instead of removing them?

Edit: There's a code path that loads block styles inline with file_get_contents:

wp_add_inline_style( "wp-block-{$block_name}", file_get_contents( $path ) );

I wonder if we can disable it.

@gziolo
Copy link
Member

gziolo commented Oct 14, 2022

Would the following work:

add_filter( 'styles_inline_size_limit', '__return_zero' );

See more in:

https://make.wordpress.org/core/2021/07/01/block-styles-loading-enhancements-in-wordpress-5-8/

@adamziel adamziel force-pushed the try/no-static-files-in-build branch from bcb141b to 0cb7e62 Compare October 14, 2022 23:40
@adamziel
Copy link
Collaborator

adamziel commented Oct 14, 2022

I rebased and kept the block styles in the bundle in 0cb7e62. The final .data file size is now 13MB so not too bar. I will also try the alternative proposed by @gziolo

@adamziel
Copy link
Collaborator

adamziel commented Oct 14, 2022

@gziolo Unfortunately it didn't help :-(

My guess is the filter works for the styles referenced by path that opt-in to the optional inlining:

wp_style_add_data( $style_handle, 'path', $file_path );

However, the inline block styles are inlined using their content – there is no URL to fall back to when the filter is __return_zero:

wp_add_inline_style( "wp-block-{$block_name}", file_get_contents( $path ) );

13M is a huge improvement compared to the 44M before this PR so I'd say let's merge and keep iterating in follow-up PRs :-)

@adamziel adamziel merged commit 2ebedc5 into trunk Oct 14, 2022
@adamziel adamziel deleted the try/no-static-files-in-build branch October 14, 2022 23:51
@adamziel adamziel mentioned this pull request Oct 14, 2022
10 tasks
@adamziel
Copy link
Collaborator

adamziel commented Oct 14, 2022

Oh no, I removed dist-web/wp.data from the repo! Just restored in f521cba. We don't need to keep the pre-built .js file in the repo. At the same time, there's a huge value in shipping pre-built PHP and WP as both require non-trivial setup to build locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider disabling script/style concatenation
3 participants