-
Notifications
You must be signed in to change notification settings - Fork 800
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Blocks: various optimizations #39734
base: trunk
Are you sure you want to change the base?
Conversation
…jetpack into add/performance-improvements
…jetpack into add/performance-improvements
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
…jetpack into add/performance-improvements
The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good and tests well. I only have some minor, non-blocking remarks, that could be addressed in a follow-up PR if you prefer.
Jetpack_Gutenberg::set_extension_available( 'ai-general-purpose-image-generator' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-assistant-site-logo-support' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-title-optimization-keywords-support' ); | ||
if ( apply_filters( 'breve_enabled', true ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe that's a good opportunity to add a docblock for this filter?
Jetpack_Gutenberg::set_extension_available( 'ai-assistant-support' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-assistant-form-support' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-content-lens' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-assistant-backend-prompts' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-assistant-usage-panel' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-featured-image-generator' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-title-optimization' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-assistant-experimental-image-generation-support' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-general-purpose-image-generator' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-assistant-site-logo-support' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-title-optimization-keywords-support' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think you could order them alphabetically? It would be easier to quickly parse, I think.
Jetpack_Gutenberg::set_extension_available( 'ai-assistant-support' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-assistant-form-support' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-content-lens' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-assistant-backend-prompts' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-assistant-usage-panel' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-featured-image-generator' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-title-optimization' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-assistant-experimental-image-generation-support' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-general-purpose-image-generator' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-assistant-site-logo-support' ); | ||
Jetpack_Gutenberg::set_extension_available( 'ai-title-optimization-keywords-support' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now have an extra one, ai-list-to-table-transform
, with an extra filter:
jetpack/projects/plugins/jetpack/extensions/blocks/ai-assistant/ai-assistant.php
Lines 82 to 94 in d4e8ec5
/** | |
* Register the `ai-list-to-table-transform` extension. | |
*/ | |
add_action( | |
'jetpack_register_gutenberg_extensions', | |
function () { | |
if ( apply_filters( 'jetpack_ai_enabled', true ) && | |
apply_filters( 'list_to_table_transform_enabled', false ) | |
) { | |
\Jetpack_Gutenberg::set_extension_available( 'ai-list-to-table-transform' ); | |
} | |
} | |
); |
Maybe you can add it to the same function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked, but I wonder if there's any chance of renaming the filter to be prefixed with jetpack_
while we're in there.
Jetpack_Gutenberg::load_independent_blocks(); | ||
Jetpack_Gutenberg::load_block_editor_extensions(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those 2 methods were hooked into plugins_loaded
in class.jetpack.php
. If we can switch them to being loaded in modules/blocks.php
(does this mean loading them on after_setup_theme
on the frontend), can we do the same with register_block_metadata_collection
below?
Proposed changes:
jetpack/
from block names continually. Instead of doing those runs, standardize all blocks to only have their unprefixed version e.g.donations
so we only need to add it when we need it and we don't need to waste cycles on figuring out if we need to add/remove it. This simplifies the block loading tooling. We still check for the prefix in the userland space.index.json
for awhile. We can simplify the code by not needing the name value to be passed and assume it is already present.index.json
- we can store this in memory after first read to avoid multiple file reads.Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
do_action( 'qm/start', 'jp_ind_blocks' ); // phpcs:ignore
anddo_action( 'qm/stop', 'jp_ind_blocks' ); // phpcs:ignore
at the start and end of the Jetpack_Gutenberg::load_indepedent_blocks function to measure the difference.For my testing, there is some difference, but not enough for me to say it is fully optimized. Need to rework a lot of this, I think, to make it more performant.