Skip to content
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
merged 16 commits into from
May 4, 2023
+15 −1
Merged

Use 'enqueue_block_assets' action when is available #9332

Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
5aa74cd
use 'enqueue_block_assets' is available
gigitux May 3, 2023
945ecf1
Merge branch 'trunk' into 9331-with-gutenberg-trunk-the-images-arent-…
gigitux May 3, 2023
9bf6adb
Merge branch 'trunk' into 9331-with-gutenberg-trunk-the-images-arent-…
gigitux May 3, 2023
8971146
Merge branch 'trunk' into 9331-with-gutenberg-trunk-the-images-arent-…
gigitux May 4, 2023
ff1c51c
enqueue in a different way
gigitux May 4, 2023
090f58b
Run init if not wp_admin() to register AssetDataRegistry scripts
tjcafferkey May 4, 2023
801ca13
try now
gigitux May 4, 2023
0577d0c
update enqueue block assets
nefeline May 4, 2023
e366216
Run init action on non-site editor pages
tjcafferkey May 4, 2023
c7abac1
Run init action on non-site editor pages
tjcafferkey May 4, 2023
ee15cdb
try now
gigitux May 4, 2023
784fa46
Enqueue wc-settings early
nefeline May 4, 2023
c84d388
Run init action on non-site editor pages
tjcafferkey May 4, 2023
850ab6e
Refactor
tjcafferkey May 4, 2023
aaa2929
Ditch enqueue of 'wp-settings'
nefeline May 4, 2023
7db5540
Ditch the pattern
nefeline May 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion src/Assets/AssetDataRegistry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it should be okay when restricted to the site editor 🤔

Copy link
Contributor

@nerrad nerrad May 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@gigitux @nefeline I am happy to pick this refactor up instead of waiting for cooldown.

} else {
add_action( 'init', array( $this, 'register_data_script' ) );
}
add_action( 'wp_print_footer_scripts', array( $this, 'enqueue_asset_data' ), 2 );
add_action( 'admin_print_footer_scripts', array( $this, 'enqueue_asset_data' ), 2 );
}

/**
* Checks if the current URL is the Site Editor.
*
* @return boolean
*/
protected function is_site_editor() {
$url_path = isset( $_SERVER['REQUEST_URI'] ) ? sanitize_text_field( wp_unslash( $_SERVER['REQUEST_URI'] ) ) : '';
return str_contains( $url_path, 'site-editor.php' );
}

/**
* Exposes core data via the wcSettings global. This data is shared throughout the client.
*
Expand Down