Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#53505 closed defect (bug) (fixed)

`wp_should_load_separate_core_block_assets()` is confusing

Reported by: desrosj's profile desrosj Owned by:
Milestone: 5.8 Priority: normal
Severity: normal Version: 5.8
Component: Script Loader Keywords: has-patch
Focuses: Cc:

Description

The wp_should_load_separate_core_block_assets() function is confusing. There is not enough context to understand what "separate" means.

"Separate" means that the CSS assets will be loaded individually instead of one combined asset. It's also confusing which outcome true or false results in. This extends to the filter as well.

See the discussion on Slack.

Attachments (2)

53505.diff (1.1 KB) - added by SergeyBiryukov 4 years ago.
53505.2.diff (2.2 KB) - added by desrosj 4 years ago.

Download all attachments as: .zip

Change History (16)

This ticket was mentioned in Slack in #core-editor by desrosj. View the logs.


4 years ago

#2 @aristath
4 years ago

I agree that the function-name is not perfect and could be improved, suggestions are welcomed.
Since I see in a few Slack conversations & tickets there's some confusion regarding what the function does, I'll post some details below cause it's admittedly not easy.

Currently, the implementation deals mostly with stylesheets but the plan is to expand the behavior to scripts - which is why it's using the term assets and not styles in its name.

The goal is to have sites that only load the scripts and styles they need instead of monolithic, large assets, therefore improving performance & sustainability.

What the function does:

Editor

On the editor it doesn't really make a difference and things work as they did prior to 5.8. Nothin changes there for backwards-compatibility reasons.

Frontend

On the frontend:

  • If wp_should_load_separate_core_block_assets is true:
    • style.css becomes common.css (does not contain block styles)
    • Core blocks only load their styles on-render, and each block loads its own, individual stylesheet.
    • 3rd-party blocks only load their styles when rendered.
  • If wp_should_load_separate_core_block_assets is false: Everything works exactly as it did in WP 5.7
    • All core block styles get loaded regardless of whether they are rendered in a page or not, because they are all part of the style.css file.
    • 3rd-party blocks always get enqueued regardless of whether they are rendered or not.

Setting wp_should_load_separate_core_block_assets to false (which is the default) doesn't change anything, and things work exactly the way they did prior to WP 5.8. The reason this was done is compatibility with themes, as setting the value to true changes the loading order of styles and themes may need to change some of their styles to make them more specific. So this is opt-in for classic themes, as they will need to test it and possibly tweak some styles.


Backtracking where the function is used and how:

register_block_style_handle: (Runs both in frontend and backend)

Skips registering styles for core-only blocks when should_load_separate_core_block_assets is false. Since wp_should_load_separate_core_block_assets checks for is_admin(), core blocks won't register their separate styles in the editor and therefore separate styles won't be enqueued, making this change only apply to the frontend.

wp_default_styles: (Runs both in frontend and backend)

Changes the URL of the block-library stylesheet from style.css to common.css.
Since wp_should_load_separate_core_block_assets checks for is_admin(), this change will only happen on the frontend.

wp_enqueue_registered_block_scripts_and_styles: (Runs both in frontend and backend)

  • Frontend
    • If wp_should_load_separate_core_block_assets is true, then styles both for core & 3rd-party blocks get enqueued on render_block. The check in this function ensures that we exit early and therefore these styles won't always get enqueued, but instead will only be enqueued on-render.
    • If wp_should_load_separate_core_block_assets is false, then styles get enqueued. Core blocks haven't been registered because they get skipped in register_block_style_handle so only 3rd-party block styles get enqueued. This makes 3rd-party blocks styles always be enqueued - just like it was prior to WP5.8.
  • Editor
    • wp_should_load_separate_core_block_assets is false, so only 3rd-party block styles get enqueued. Core block styles are included in the main style.css file - just like it was prior to WP5.8.

#3 @desrosj
4 years ago

Thanks for these details, @aristath! I am still giving this one some thought, and will circle back with what I come up with a bit later today.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


4 years ago

#5 @JeffPaul
4 years ago

  • Keywords needs-patch added

This ticket was mentioned in PR #1444 on WordPress/wordpress-develop by aaronjorbin.


4 years ago
#6

  • Keywords has-patch added; needs-patch removed

See https://core.trac.wordpress.org/ticket/53505

Props aristath who gave a fantastic explanation of how and why this function exists. I synthasysed that into a more docs friendly paragraph and added a couple of @see for places that use this function.

Trac ticket:

#7 @jorbin
4 years ago

Took a pass at turning @aristath's excellent explanation into something that will fit in an inline doc

@SergeyBiryukov
4 years ago

#8 @SergeyBiryukov
4 years ago

In 51256:

Docs: Improve documentation for wp_should_load_separate_core_block_assets().

This should clarify the purpose of the function and what effect it has on block styles loading.

Props aristath, desrosj, jorbin, SergeyBiryukov.
See #53505.

@desrosj
4 years ago

#9 follow-up: @desrosj
4 years ago

Thanks everyone! This is much better. I still don't love the function name, but a better alternative is not coming to mind.

53505.2.diff has some small tweaks to [51256] to refer to assets more instead of stylesheet as the plan is to use this for scripts as well. It also copies some of the improvements already made to the should_load_separate_core_block_assets filter docblock.

#10 in reply to: ↑ 9 @SergeyBiryukov
4 years ago

Replying to desrosj:

53505.2.diff has some small tweaks to [51256] to refer to assets more instead of stylesheet as the plan is to use this for scripts as well.

Looks good to me :)

My only concern is that mentioning scripts here is technically not quite correct yet, but I guess that will fix itself when the function is also used for scripts in a future release.

#11 @jorbin
4 years ago

In 51263:

Docs: Further Improve documentation for wp_should_load_separate_core_block_assets().

In [51256], the documentation was improved, this further improves it. As scripts are not yet a part of what this function does, they are not mentioned explicitly. Assets is used so that the docs don't need a lot of improvements when scripts do come into play..

Props desrosj, SergeyBiryukov, Jorbin.
See #53505.

#12 @desrosj
4 years ago

  • Resolution set to fixed
  • Status changed from new to closed

With RC later today, I'm going to close this one out. A better function/hook name has not been suggested, but the documentation is much more clear now.

#13 @desrosj
4 years ago

  • Version set to trunk
Note: See TracTickets for help on using tickets.