Skip to content
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

Lazy load varients #5825

Closed

Conversation

spacedmonkey
Copy link
Member

Improve on #5718, with improved B/C.

Trac ticket: https://core.trac.wordpress.org/ticket/59969


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

Thanks @spacedmonkey. This is fairly similar to what I had in mind. A few questions:

  1. This implementation prefers that the variation callback be run each time the property is needed. However, this means that there is potential of an expensive operation to be run multiple times during the same run. It's maybe unlikely that this will actually occur, but is something worth considering. We could rely on the magic getter to call the get_variations() callback only if the variations property isn't already set (which is what PHP will do naturally) rather than trying to prefer the callback each time you need to access variations.
  2. I'm curious if we can avoid loading variations in get_block_editor_server_block_settings entirely? This would speed up the initial load of the editor if we can hydrate those values from the REST API instead.

@spacedmonkey
Copy link
Member Author

  1. This implementation prefers that the variation callback be run each time the property is needed. However, this means that there is potential of an expensive operation to be run multiple times during the same run. It's maybe unlikely that this will actually occur, but is something worth considering. We could rely on the magic getter to call the get_variations() callback only if the variations property isn't already set (which is what PHP will do naturally) rather than trying to prefer the callback each time you need to access variations.

I did consider this. Calling the callback once and then setting a flag saying it has be called, then "caching" the value in the varianets property. This is do-able. But I wonder if the callback might return dynamitic value, making it not cachable.

  1. I'm curious if we can avoid loading variations in get_block_editor_server_block_settings entirely? This would speed up the initial load of the editor if we can hydrate those values from the REST API instead.

Also considered. Personally I think it is above my paid grade when it comes to JS changes, I would need someone from gutenberg team to help. I have had a ticket WordPress/gutenberg#22812 since 2020, on this very topic. I wonder if @Mamaduka could support on this one.

Thanks for your feedback @joemcgill . I don't have enough time to get this one across the line myself. Please take this code example and fork / apply to your own PRs.

@Mamaduka
Copy link
Member

Mamaduka commented Jan 4, 2024

I'm happy to help out with the JS part of the code for WordPress/gutenberg#22812, but I don't think it's a blocker for lazy loading variation improvements.

The implementation-wise block_editor_rest_api_preload() isn't much different from what we're already doing, so I'm wondering if there will be any performance benefits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants