Refactor block types to include script registration and integration classes #3829
Conversation
mikejolley
added
status: needs review
category: extensibility
mikejolley
force-pushed
the
add/abstract-integration-classes-3532
branch
from
5f3a3aa to
c5cd9ab
Compare
|
Size Change: +39 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
| * | ||
| * @param IntegrationRegistry $this Instance of the IntegrationRegistry class which exposes the IntegrationRegistry::register() method. | ||
| */ | ||
| do_action( 'woocommerce_blocks_' . $this->registry_identifier . '_registration', $this ); |
There was a problem hiding this comment.
Could we only send ::register() and not the entire class instance?
There was a problem hiding this comment.
This is matching the pattern in the Payments Registry. There are some other useful methods like unregistration that may be needed.
|
I can't really follow all the changes since these are 3 different things at the same time. The only thing I was able to spot were simple nitpicks. I will just try to test as much as possible. What is the impact on existing third-party integrations if they are using AbstractBlock? The class constructor signature was changed. Is this a public API? We have it documented in the |
|
I have tried to test as much as I could, and it all looks good. |
github-actions
bot
added
status: ready to merge
and removed
status: needs review
labels
@budzanowski I wasn't totally sure on this. Yes it's "public" because it's accessible, but it's something internal to our blocks and has not been documented as an extendibility API or anything such as this. I would argue it's internal, even if the scope requires it to be public. |
|
Thanks for the review @budzanowski 👍🏻 @senadir did you also get a chance to review this? @nerrad did you have any thoughts on #3829 (comment)? |
mikejolley
force-pushed
the
add/abstract-integration-classes-3532
branch
from
b6ec400 to
087cb82
Compare
nerrad
added
the
needs: dev note
|
Dev note: An important note that internally, this release has modified how |
This is a fairly hefty PR which refactors our Block Type classes (and makes some changes to the Payments Integration classes).
1. Removes editor script registration from the Assets class to the block type classes
This keeps block type definitions together with the script registration, avoids the need to do additional checks for build type when registering assets (because blocks are not registered if they do not support the current build), and allows each block type to define it's own dependencies.
2. Abstracts
PaymentMethodRegistryandPaymentMethodTypeInterfacewithIntegrationRegistryandIntegrationInterfaceSince we needed something similar for other integrations (block types) I created
IntegrationRegistryandIntegrationInterfaceand moved the code that can be reused to those.AbstractPaymentMethodTypetakes care of the differences and this seems to be used by WC Pay etc so shouldn't break anything.3.
AbstractBlockrefactorThis class will now:
IntegrationRegistrybased on the block name, and with this append registered dependencies and data whenever assets are registered or enqueued.@senadir before this is merged I want to update subscriptions with it to ensure it covers all use cases. We will hook in via:
woocommerce_blocks_{registry_identifier}_registrationis the hook where 3rd parties can register themselves. In the case of subscriptions it will be both:Fixes #3532
How to test the changes in this Pull Request:
This touches on quite a lot of code so we need to ensure:
Changelog
Dev note:
An important note that internally, this release has modified how
AbstractBlock(the base class for all of our blocks) functions, and how it loads assets.AbstractBlockis internal to this project and does not seem like something that would ever need to be extended by 3rd parties, but note if you are doing so for whatever reason, your implementation would need to be updated to match.