Don't load wc blocks data on every route. #3864
Conversation
|
Size Change: +1.52 kB (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
…thub.com:woocommerce/woocommerce-gutenberg-products-block into fix/3748-dont-load-wc_blocks_data-on-every-route
This was used for wc-admin <= v0.19.0 which is 1.5year old at this moment.
budzanowski
changed the title
|
I have intended to rebase but ended up with merge. Fingers automation 😞 |
budzanowski
changed the title
| 'shippingMethodsExist', | ||
| false | ||
| ); | ||
| const blockConstants = getSetting( 'block_constants', {} ); |
There was a problem hiding this comment.
This is a breaking change, I know we're not using this publicly yet but it should be highlighted at least.
For example, I used getSetting( 'displayCartPricesIncludingTax', false ); which is not longer available.
What's the reason for moving all data under block_constants?
There was a problem hiding this comment.
With the refactor each block tells the assets registry that it needs the constants. Until a block rendered happens the AssetsRegistery will not trigger the constants creation. This was the sole purpose of the refactor - no to trigger the wc_data_creation on every route. And now with that in mind the reason for the block_constant: since this is no longer global and each block does it is own registering we need a way to easily check if the data was already requested - so we will be able to avoid unnecessary multiple registers. This is managed by the AbstractBlock:
// Enqueue common data.
if ( ! $this->asset_data_registry->exists( 'block_constants' ) ) {
$this->asset_data_registry->add( 'block_constants', [ self::class, 'get_wc_block_data' ] );
}We have only one key that we need to check. Any other dynamic approach way would require adjusting the AssetsDataRegistery framework for this special case of blocks common data which I think we should not do.
There was a problem hiding this comment.
I agree with Nadir here, this is a pretty significant breaking change and should be reconsidered. I'm not sure I agree with the solution in this refactor. I agree there needs to be a way for each block to register what data it needs (without triggering the error from duplicate keys) but I would prefer to see that happen at the block level. To avoid having to check if the data is available before registering (via AssetsDataRegistry::exists) - we could add a new helper method addIfNotExists or something like that which absorbs the necessary exists check and avoids registering duplicate data on the same key.
Then blocks that share data could potentially do the registration via a method on the abstract block class.
Some variation of the above should work and allows us to preserve back compat.
There was a problem hiding this comment.
@budzanowski I realize you won't be following up on this given your rotation, but just thought I'd leave my review here. I might pick this up this week so we can get it merged. Thanks for the work you did on this!
| import { getSetting } from '@woocommerce/settings'; | ||
| import { WORD_COUNT_TYPE } from '@woocommerce/block-settings'; |
There was a problem hiding this comment.
Let's switch back to use getSetting here. I'd like us to move away from using block-settings and use getSetting instead. That helps make the dependency much clearer.
This comment applies to all the other places modified to use getSetting.
| 'shippingMethodsExist', | ||
| false | ||
| ); | ||
| const blockConstants = getSetting( 'block_constants', {} ); |
There was a problem hiding this comment.
I agree with Nadir here, this is a pretty significant breaking change and should be reconsidered. I'm not sure I agree with the solution in this refactor. I agree there needs to be a way for each block to register what data it needs (without triggering the error from duplicate keys) but I would prefer to see that happen at the block level. To avoid having to check if the data is available before registering (via AssetsDataRegistry::exists) - we could add a new helper method addIfNotExists or something like that which absorbs the necessary exists check and avoids registering duplicate data on the same key.
Then blocks that share data could potentially do the registration via a method on the abstract block class.
Some variation of the above should work and allows us to preserve back compat.
| $message = __( | ||
| 'The filter should not be used for assets registration. Please read woocommerce-gutenberg-products-block/docs/contributors/block-assets.md for more information on how to proceed.', | ||
| 'woo-gutenberg-products-block' | ||
| ); | ||
|
|
||
| $settings = apply_filters_deprecated( 'woocommerce_shared_settings', [ $this->data ], '4.6.0', '', $message ); |
| $asset_api = $container->get( AssetApi::class ); | ||
| $load_back_compat = defined( 'WC_ADMIN_VERSION_NUMBER' ) | ||
| && version_compare( WC_ADMIN_VERSION_NUMBER, '0.19.0', '<=' ); | ||
| return $load_back_compat | ||
| ? new BackCompatAssetDataRegistry( $asset_api ) | ||
| : new AssetDataRegistry( $asset_api ); | ||
| $asset_api = $container->get( AssetApi::class ); | ||
| return new AssetDataRegistry( $asset_api ); |
Fixes: #3748
There are a couple of connected but different things happening in this PR:
BackCompatAssetRegistry: this was used for WC Admin <= v0.19.0. This is an 18 months old version so this should be safe to remove.woocommerce_shared_settingsfilter. In the new approach, the common data is registered by each block ( previously checking if it was not already registered ) in AbstractBlock::enqueue_data. This is managed by the default render_callback. This ensures that the data creation function will be called only if Gutenberg will render a block.Testing:
This reorganizes how data required by blocks are loaded. Therefore this requires a rather thorough smoke testing.
The only truly visible item from the outside should be the filter deprecation. All other items are internal refactor and are skipped from the changelog.
Changelog