This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Use 'enqueue_block_assets' action when is available #9332
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gigitux
added
type: bug
woocommercebot
requested review from
a team and
kmanijak
and removed request for
a team
Remove this check when WordPress 6.3 is the minimum suppo...Remove this check when WordPress 6.3 is the minimum supported version.
woocommerce-blocks/src/Assets/AssetDataRegistry.php Lines 69 to 73 in 5aa74cd 🚀 This comment was generated by the automations bot based on a
|
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the TypeScript Errors Report
🎉 🎉 This PR does not introduce new TS errors. |
|
Size Change: 0 B Total Size: 1.11 MB ℹ️ View Unchanged
|
tjcafferkey
approved these changes
May 3, 2023
…loaded-on-the-site-editor
…loaded-on-the-site-editor
…loaded-on-the-site-editor
gigitux
force-pushed
the
9331-with-gutenberg-trunk-the-images-arent-loaded-on-the-site-editor
branch
from
20cf6ad to
8971146
Compare
gigitux
deleted the
9331-with-gutenberg-trunk-the-images-arent-loaded-on-the-site-editor
branch
nerrad
reviewed
May 4, 2023
| @@ -66,11 +66,25 @@ public function __construct( Api $asset_api ) { | |||
| * Hook into WP asset registration for enqueueing asset data. | |||
| */ | |||
| protected function init() { | |||
| add_action( 'init', array( $this, 'register_data_script' ) ); | |||
| if ( $this->is_site_editor() ) { | |||
| add_action( 'enqueue_block_editor_assets', array( $this, 'register_data_script' ) ); | |||
There was a problem hiding this comment.
This is dangerous because there may be other Woo Extensions using the AssetDataRegistry which aren't explicitly using it for blocks. Have you double-checked this?
There was a problem hiding this comment.
I guess it should be okay when restricted to the site editor 🤔
There was a problem hiding this comment.
Still, I wonder if in a followup iteration we should expose a separate API on AssetDataRegistry that is explicitly for blocks to register their asset data rather (and thus enqueued differently) than accounting for multiple locations here. That would help with future maintenance as well if things change.
There was a problem hiding this comment.
there may be other Woo Extensions using the AssetDataRegistry which aren't explicitly using it for blocks. Have you double-checked this?
I guess it should be okay when restricted to the site editor
Yeah, this has only really changed within the context of the site editor. And are we aware of any plugins (of our own or third party) that pass data to the site editor in a non-blocks context? It seems like a bit of an edge case but definitely worth thinking about/a look into.
There was a problem hiding this comment.
Still, I wonder if in a followup iteration we should expose a separate API on AssetDataRegistry that is explicitly for blocks to register their asset data rather (and thus enqueued differently) than accounting for multiple locations here
Agree on this! This is just a patch. We will revisit the entire process during the next cooldown!
There was a problem hiding this comment.
This is dangerous because there may be other Woo Extensions using the AssetDataRegistry which aren't explicitly using it for blocks. Have you double-checked this?
I guess it should be okay when restricted to the site editor 🤔
That's a good point: I think so, but IMO it's hard to be 100% sure at this point without any further investigation. We have #9354 as a FU task for a permanent fix.
There was a problem hiding this comment.
@tjcafferkey @gigitux @nerrad do you think we should prioritize revising this before releasing the patch, just to make sure we won't end up breaking things elsewhere, or are we a-ok with keeping the patch as-is and revisiting later?
There was a problem hiding this comment.
I would release as-is, but instead of waiting for the cooldown, work on a proper fix now. In this way, we can:
- Apply the new changes if we break something.
- Work on a proper solution without the rush.
There was a problem hiding this comment.
I tend to be more conservative in such cases so my vote would be for spending some extra time investigating alternatives & possible consequences before releasing, but if most of us are comfortable with releasing as-is I agree with your proposed approach @gigitux
thealexandrelara
pushed a commit
that referenced
this pull request
May 4, 2023
thealexandrelara
added a commit
that referenced
this pull request
May 4, 2023
thealexandrelara
added a commit
that referenced
this pull request
May 4, 2023
gigitux
added a commit
that referenced
this pull request
May 5, 2023
* Empty commit for release pull request * Add 10.0.3 changelog * Update versions to 10.0.3 * Fix image editor in Featured Product/Category blocks on WP 6.2 (#9142) * Add 10.0.3 testing steps * Empty commit for release pull request * Check that the customized fallback template is archive-product before unsetting the source property (#9330) * use 'enqueue_block_assets' is available (#9332) * Remove esc_url() on self generated link to edit the Mini Cart template since it gets escaped in JS (#9348) * Add changelog entries to readme.txt * Update version number in several files * Add testing notes for the release * Add #9332 to testing notes * Add testing on frontend for #9332 --------- Co-authored-by: github-actions <[email protected]> Co-authored-by: Albert Juhé Lluveras <[email protected]> Co-authored-by: Tom Cafferkey <[email protected]> Co-authored-by: Luigi Teschio <[email protected]> Co-authored-by: Alexandre Lara <[email protected]>
mikejolley
added a commit
that referenced
this pull request
May 17, 2023
…epending on admin vs frontend
4 tasks
mikejolley
added a commit
that referenced
this pull request
May 18, 2023
…epending on admin vs frontend (#9495)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the newer version of Gutenberg (and WordPress), there is a dedicated action to enqueue the assets of blocks on the Site Editor (WordPress/gutenberg#49655). This PR uses the new action.WordPress/gutenberg#49655 changed how assets should be enqueued on the Site Editor. This PR is just a dirty patch to fix #9331, but we will revisit the entire process in the next cooldown. #9354
🤖 Generated by Copilot at 945ecf1
Summary
🆕🚀🐛
Hook
register_data_scripttoenqueue_block_assetsif available, orinitotherwise, for compatibility and performance reasons. This is part of a pull request to improve asset data loading.Walkthrough
register_data_scripttoenqueue_block_assetsif available, orinitotherwise, to improve data loading performance and compatibility (link)Fixes #9331
Testing
Automated Tests
User Facing Testing
WooCommerce Visibility