The release ZIP for this PR is accessible via:

https://wcblocks.wpcomstaging.com/wp-content/uploads/woocommerce-gutenberg-products-block-11664.zip

Script Dependencies Report

The compare-assets action has detected some changed script dependencies between this branch and trunk. Please review and confirm the following are correct before merging.

This comment was automatically generated by the ./github/compare-assets action.

TypeScript Errors Report

• Files with errors: 575
• Total errors: 2487

🎉 🎉 This PR does not introduce new TS errors.

_{comments-aggregator}

Size Change: +6.61 kB (0%)

Total Size: 1.59 MB

_{compressed-size-action}

I noticed that with this approach we need to define the blocks with registration restrictions in blocks-with-restriction.ts. Do you think it would be possible to create a util wrapping registerBlockType() which takes care of that? So we would only need to call that util (ie: registerBlockTypeWithRestrictions()) when registering the block instead of having to modify blocks-with-restriction.ts if we ever need to do this for other blocks.

Thank you for reviewing and suggesting this approach! Let me provide more context about why this approach is being used here.

Currently, we have the assets/js/atomic/utils/register-block-single-product-template.ts, which works as a wrapper for the registerBlockType() and registerBlockVariation() functions. One of the issues we encounter when using this wrapper is that, for each component that utilizes it, we subscribe to the data store (core/edit-site and core/edit-post). Consequently, every time the store is updated, this logic is triggered for each subscriber.

We currently have approximately 8 blocks using this approach. Since each instance of registerBlockSingleProductTemplate is isolated from the others, they are triggered independently, but they update the store simultaneously. This can lead to racing conditions (#9090).

The idea behind using this approach is to establish a single source of truth responsible for monitoring and responding to page/post or template changes, as well as registering/unregistering blocks based on the defined restrictions. This approach also prevents racing conditions since there is a single point responsible for block registration/unregistration. Another advantage is improved performance, as we only have one subscriber to the store, which triggers the logic for managing the blocks only when merchants go from one post/page to another or from one template to another.

Regarding your suggestion, what changes do you think we could make to retain these benefits while avoiding the use of a separate file for block restrictions?

By the way, it might be a good idea to add some e2e tests to make sure we don't introduce regressions on this feature in the future. What do you think?

Sure! I'll be adding some e2e tests for the Product Gallery block to make sure the block is only available inside the Single Product template and not on the posts/pages.

I also had similar concerns as Albert. Is there a way to add metadata to the Product Gallery block and when it gets registered through the wrapper, it will pick up on the restrictions instead of using that restriction file?

The idea behind using this approach is to establish a single source of truth responsible for monitoring and responding to page/post or template changes, as well as registering/unregistering blocks based on the defined restrictions. This approach also prevents racing conditions since there is a single point responsible for block registration/unregistration. Another advantage is improved performance, as we only have one subscriber to the store, which triggers the logic for managing the blocks only when merchants go from one post/page to another or from one template to another.

That makes a lot of sense, @thealexandrelara! Thanks for the detailed explanation.

I was thinking about something along these lines:

export const registerBlockTypeWithRestrictions = (
	blockMetadata,
	blockSettings,
	{
		allowedTemplates,
		allowedTemplateParts,
		availableInPostOrPageEditor,
		isVariationBlock,
	}
) => {
	BLOCKS_WITH_RESTRICTION[ blockMetadata.name ] = {
		blockMetadata,
		blockSettings,
		allowedTemplates,
		allowedTemplateParts,
		availableInPostOrPageEditor,
		isVariationBlock,
	};
	registerBlockType( blockMetadata, blockSettings );
};

Which then could be called like this:

registerBlockTypeWithRestrictions(
	productGalleryBlockMetadata,
	ProductGalleryBlockSettings,
	{
		allowedTemplates: {
			'single-product': true,
		},
		allowedTemplateParts: {
			'product-gallery': true,
		},
		availableInPostOrPageEditor: false,
		isVariationBlock: false,
	}
);

I didn't test it, and it would probably require some internal refactoring, as BLOCKS_WITH_RESTRICTION would be a dynamic constant. So there might be some blockers that I'm not aware of. In any case, I wanted to share it as I think it would make the API much easier to work with.

Besides that, everything looks good to me. 👌

Thank you for your suggestions, it would be awesome to have this API and it would make much easier to maintain. I was testing your suggestions for improving the API and creating a wrapper around the registerBlockType. However, it appears to be more complex than I initially expected. The issue is that the BlocksRegistrationManager only runs when there's a template or page change. When you first load the page, the function runs as soon as the page loads. However, the blocks are registered later. Since the block isn't registered before the manager runs, the manager doesn't know when it should remove the block, making it available in places where it shouldn't (when loading the page for the first time).

I'll be AFK for the next few weeks, but this PR will be essential for the Product Gallery block. I believe I can improve this API when we start migrating other blocks to use the BlocksRegistrationManager. The other option would be for someone else to continue working on this and improve the API before merging it. What do you think?

P.S.: I've just enabled auto-merge, so once this PR is approved it will be automatically merge.

@roykho I pushed some additional commits to this PR. Would you mind taking a second look to make sure I didn't mess anything up? 🙏

@Aljullu - it's working correctly thanks for the additions.