Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Create AssetsController and BlockTypesController (replaces Assets.php and Library.php) #4094

Merged
merged 9 commits into from
Apr 26, 2021

Conversation

mikejolley
Copy link
Member

@mikejolley mikejolley commented Apr 22, 2021

Refactors Assets and Library with dependency injection. Closes #3875 and fixes #3868

I've marked this as in need of a dev note since we're removing classes, however, they are internal so unlikely to be used directly. cc @nerrad

Will add inline notes for other changes.

How to test the changes in this Pull Request:

General smoke test to ensure classes are still loaded. Check all block types are registered and existing blocks load within admin and frontend.

Dev Note

Two internal classes (Automattic\WooCommerce\Blocks\Assets and Automattic\WooCommerce\Blocks\Library) have been deprecated in this release, with functionality being moved to AssetsController and BlockTypesController. Since these were internal bootstrapping classes we don't anticipate any external usage, but noting that these classes will be removed in a future release of Blocks.

Changelog

Introduced AssetsController and BlockTypesController classes (which replace Assets.php and Library.php).

@mikejolley mikejolley added status: needs review needs: dev note PR that has some text that needs to be included in the release notes. labels Apr 22, 2021
@mikejolley mikejolley requested a review from nerrad April 22, 2021 15:48
@mikejolley mikejolley requested a review from a team as a code owner April 22, 2021 15:48
@mikejolley mikejolley self-assigned this Apr 22, 2021
@github-actions
Copy link
Contributor

Remove fix to load our stylesheets after editor CSS. See ...

Remove fix to load our stylesheets after editor CSS. See #3068 and #3898 for the rationale of this fix. It should be no github.com/WordPress/gutenberg/issues/20797).


https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/b14625accbc66e08516a8608e38530da9007dfef/src/AssetsController.php#L87-L100

🚀 This comment was generated by the automations bot based on a todo comment in b14625a in #4094. cc @mikejolley

* @param string|array $classes Array or string of CSS classnames.
* @return string|array Modified classnames.
*/
public function add_theme_body_class( $classes ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Note I combined the 2 functions into one which handles $classes based on type instead of 2 functions for similar logic.

add_action( 'init', array( __CLASS__, 'register_blocks' ) );
add_action( 'init', array( __CLASS__, 'define_tables' ) );
protected function init() {
add_action( 'init', array( $this, 'register_blocks' ) );
Copy link
Member Author

Choose a reason for hiding this comment

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

Note, we don't need to define tables any more now that the stock table is in core.

* @param string $handle Optional. Provided if the handle should be different than the script name. `wc-` prefix automatically added.
* @param array $dependencies Optional. An array of registered script handles this script depends on. Default empty array.
*/
public function register_block_script( $script_name, $handle = '', $dependencies = [] ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Note; I removed these as they have been deprecated for several versions and are not being used internally.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 22, 2021

Size Change: +2.5 kB (0%)

Total Size: 1.15 MB

Filename Size Change
build/active-filters-frontend.js 8.1 kB +1 B (0%)
build/active-filters.js 7.65 kB -1 B (0%)
build/all-products-frontend.js 35.6 kB +522 B (+1%)
build/all-products.js 36.6 kB +386 B (+1%)
build/all-reviews.js 9.29 kB +1 B (0%)
build/atomic-block-components/add-to-cart--atomic-block-components/button--atomic-block-components/image---a7e2bb9b.js 2.33 kB +526 B (+29%) 🚨
build/atomic-block-components/add-to-cart--atomic-block-components/button.js 1.94 kB -2 B (0%)
build/atomic-block-components/add-to-cart--atomic-block-components/image--atomic-block-components/title.js 335 B -1 B (0%)
build/atomic-block-components/add-to-cart-frontend.js 8.75 kB +1 B (0%)
build/atomic-block-components/add-to-cart.js 7.84 kB +2 B (0%)
build/atomic-block-components/button-frontend.js 1.73 kB -2 B (0%)
build/atomic-block-components/image-frontend.js 1.84 kB +1 B (0%)
build/atomic-block-components/price-frontend.js 1.98 kB -1 B (0%)
build/atomic-block-components/price.js 2 kB -7 B (0%)
build/atomic-block-components/sale-badge-frontend.js 861 B +1 B (0%)
build/atomic-block-components/sale-badge.js 871 B +1 B (0%)
build/atomic-block-components/sku-frontend.js 388 B -1 B (0%)
build/atomic-block-components/sku.js 395 B +1 B (0%)
build/atomic-block-components/stock-indicator-frontend.js 569 B +1 B (0%)
build/atomic-block-components/summary-frontend.js 907 B -1 B (0%)
build/atomic-block-components/tag-list-frontend.js 466 B +1 B (0%)
build/atomic-block-components/tag-list.js 473 B +1 B (0%)
build/atomic-block-components/title.js 1.26 kB -1 B (0%)
build/attribute-filter-frontend.js 17.8 kB -1 B (0%)
build/attribute-filter.js 11.5 kB +1 B (0%)
build/blocks-checkout.js 13.2 kB +86 B (+1%)
build/cart-frontend.js 77.5 kB +192 B (0%)
build/cart.js 43.3 kB +155 B (0%)
build/checkout-frontend.js 81.8 kB +96 B (0%)
build/checkout.js 45.5 kB +37 B (0%)
build/featured-category.js 7.23 kB +1 B (0%)
build/featured-product.js 9.47 kB +9 B (0%)
build/handpicked-products.js 5.88 kB +1 B (0%)
build/price-filter-frontend.js 14.3 kB +1 B (0%)
build/price-filter.js 9.33 kB -6 B (0%)
build/price-format.js 1.47 kB +4 B (0%)
build/product-category.js 7.03 kB +1 B (0%)
build/product-new.js 6.21 kB -2 B (0%)
build/product-on-sale.js 6.64 kB +2 B (0%)
build/product-search.js 2.55 kB -3 B (0%)
build/product-tag.js 6.08 kB -3 B (0%)
build/product-top-rated.js 6.18 kB -2 B (0%)
build/products-by-attribute.js 6.98 kB +3 B (0%)
build/reviews-by-category.js 11.4 kB +1 B (0%)
build/reviews-by-product.js 12.9 kB -1 B (0%)
build/reviews-frontend.js 9.1 kB -4 B (0%)
build/single-product-frontend.js 38.6 kB +517 B (+1%)
build/single-product.js 9.71 kB -2 B (0%)
build/vendors--atomic-block-components/price-frontend.js 6.54 kB -1 B (0%)
build/vendors.js 419 kB -25 B (0%)
build/wc-blocks-data.js 10.6 kB +2 B (0%)
build/wc-blocks-middleware.js 1.49 kB +3 B (0%)
build/wc-blocks-registry.js 2.75 kB +8 B (0%)
build/wc-settings.js 2.93 kB +3 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/atomic-block-components/button.js 843 B 0 B
build/atomic-block-components/category-list-frontend.js 469 B 0 B
build/atomic-block-components/category-list.js 478 B 0 B
build/atomic-block-components/image.js 1.31 kB 0 B
build/atomic-block-components/rating-frontend.js 520 B 0 B
build/atomic-block-components/rating.js 526 B 0 B
build/atomic-block-components/stock-indicator.js 572 B 0 B
build/atomic-block-components/summary.js 914 B 0 B
build/atomic-block-components/title-frontend.js 1.41 kB 0 B
build/blocks.js 3.49 kB 0 B
build/editor-rtl.css 14.9 kB 0 B
build/editor.css 14.9 kB 0 B
build/product-best-sellers.js 6.05 kB 0 B
build/product-categories.js 3.23 kB 0 B
build/style-rtl.css 18.8 kB 0 B
build/style.css 18.8 kB 0 B
build/vendors-style-rtl.css 1.05 kB 0 B
build/vendors-style.css 1.05 kB 0 B
build/wc-blocks-google-analytics.js 1.99 kB 0 B
build/wc-payment-method-bacs.js 812 B 0 B
build/wc-payment-method-cheque.js 807 B 0 B
build/wc-payment-method-cod.js 903 B 0 B
build/wc-payment-method-paypal.js 844 B 0 B
build/wc-payment-method-stripe.js 12.3 kB 0 B
build/wc-shared-context.js 1.54 kB 0 B
build/wc-shared-hocs.js 1.72 kB 0 B

compressed-size-action

@nerrad
Copy link
Contributor

nerrad commented Apr 22, 2021

Just a quick note that I think we should still leave the classes in place with any public method and add depecrated calls/comment blocks. Once this is deployed to WooCommerce core in one version with the deprecations, the next release of blocks after that could remove the classes altogether (which will land in the next WooCommerce core release).

Some rationale here is that there are some deploy scenarios for WooCommerce core in some environments where fatals could be introduced so we have stagger actual class removal.

Copy link
Contributor

@nerrad nerrad left a comment

Choose a reason for hiding this comment

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

Code wise this looks good, thanks Mike!

I have a few items for you to look as inline comments that I'm interested in your take on. I also have this comment I made earlier that I think should be addressed.

src/AssetsController.php Show resolved Hide resolved
src/AssetsController.php Outdated Show resolved Hide resolved
src/AssetsController.php Outdated Show resolved Hide resolved
src/BlockTypesController.php Show resolved Hide resolved
src/AssetsController.php Outdated Show resolved Hide resolved
src/BlockTypesController.php Outdated Show resolved Hide resolved
src/BlockTypesController.php Show resolved Hide resolved
@mikejolley
Copy link
Member Author

@nerrad thanks, changes made. Just waiting on tests :)

Copy link
Contributor

@nerrad nerrad left a comment

Choose a reason for hiding this comment

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

Changes look good, just a minor note about the unnecessary imports in the deprecated Library class.

Good to go after tests are 🟢 and you're happy with your own testing of this.

src/Library.php Outdated
Comment on lines 4 to 8
use Automattic\WooCommerce\Blocks\BlockTypes\AtomicBlock;
use Automattic\WooCommerce\Blocks\Package;
use Automattic\WooCommerce\Blocks\Assets\AssetDataRegistry;
use Automattic\WooCommerce\Blocks\Assets\Api as AssetApi;
use Automattic\WooCommerce\Blocks\Integrations\IntegrationRegistry;
Copy link
Contributor

Choose a reason for hiding this comment

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

probably don't need all these imports now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ignore this, it was an artifact of the commits I was viewing and how the changes were being shown, I see they don't actually exist in the file.

@mikejolley mikejolley merged commit 66d01a2 into trunk Apr 26, 2021
@mikejolley mikejolley deleted the refactor/assets-3868 branch April 26, 2021 09:37
@mikejolley mikejolley added this to the 5.0.0 milestone Apr 28, 2021
@mikejolley mikejolley added type: technical debt This issue/PR represents/solves the technical debt of the project. type: refactor The issue/PR is related to refactoring. labels Apr 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs: dev note PR that has some text that needs to be included in the release notes. type: refactor The issue/PR is related to refactoring. type: technical debt This issue/PR represents/solves the technical debt of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Assets into a new initializer/loader/controller for registering block assets.
2 participants