Don't load frontend scripts in the editor #2788
Conversation
Aljullu
added
status: needs review
focus: performance
|
Size Change: +819 B (0%) Total Size: 1.59 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Code wise this looks great @Aljullu! I especially like the changes to src/Assets.php and Automattic\WooCommerce\Blocks\Assets\Api so that we preserve back-compat while treating Assets\Api as authoritative. That fixes a flaw in the existing code so 👏 .
I tested Single Product and Checkout blocks since those are the ones I was concerned about. Single Product works okay, but it looks like there is a problem with the payment method script registration for the checkout block on the frontend (it's not loading). Will you look into that please?
| @@ -40,7 +40,7 @@ public function __construct( Package $package ) { | |||
| * @return string The cache buster value to use for the given file. | |||
| */ | |||
| protected function get_file_version( $file ) { | |||
| if ( defined( 'SCRIPT_DEBUG' ) && SCRIPT_DEBUG ) { | |||
| if ( defined( 'SCRIPT_DEBUG' ) && SCRIPT_DEBUG && file_exists( $this->package->get_path() . $file ) ) { | |||
There was a problem hiding this comment.
good additional change here 👍 . Although I wonder if we should still log instances when the file doesn't exist? Should there be at least a log trail of filemtime failing because the file doesn't exist for potential troubleshooting?
There was a problem hiding this comment.
I'm not sure to understand you. You mean throwing an error if the file doesn't exist?
There was a problem hiding this comment.
No I mean error_log if it doesn't exist. Previously there'd be a php warning if filemtime was used on a file that doesn't exist and that'd be a signal that something went wrong with the build. So my comment was birthed out of wondering if we're okay with losing that signal that something isn't right with the build process.
I'm on the fence here, and I'd be okay with leaving this code as is.
| if ( version_compare( $wp_version, '5.2', '>' ) ) { | ||
| if ( version_compare( $wp_version, '5.3', '>=' ) ) { |
There was a problem hiding this comment.
I'm guessing you made the change here because it covers scenarios where the version might be 5.2.1 etc right? (in which case 💯 ).
|
Thanks for the review @nerrad!
Honestly, I'm not sure what was going on, but in 70e54dd I refactored |
| @@ -88,26 +88,31 @@ public function register_script( $handle, $relative_src, $dependencies = [], $ha | |||
| $version = $this->get_file_version( $relative_src ); | |||
| } | |||
|
|
|||
| wp_register_script( $handle, $src, $dependencies, $version, true ); | |||
| wp_register_script( $handle, $src, apply_filters( 'woocommerce_blocks_register_script_dependencies', $dependencies, $handle ), $version, true ); | |||
There was a problem hiding this comment.
I took this filter from src/Assets.php, we were inconsistently applying it in Assets but not in AssetsApi. Now that we have a unique way to register scripts, I think it makes sense to keep the filter.
There was a problem hiding this comment.
ahh that would explain it. The Payments API uses that to register the handle for payment method scripts as dependencies on certain handles (so various registration happens before the blocks load).
Fixes #1910
This PR:
register_block_scriptandget_block_asset_build_pathto the AssetApi class (134d212).register_block_scriptso it doesn't register block scripts in admin (f61fcc9).legacyfiles (e0dc45e).How to test the changes in this Pull Request:
frontendin their name.-frontend.jswas loaded.Changelog