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

Blocks: move bootstrapped block types to Redux state #53807

Merged
merged 9 commits into from
Aug 30, 2023

Conversation

jsnajdr
Copy link
Member

@jsnajdr jsnajdr commented Aug 18, 2023

This is a spinoff from the async block loading prototype (#51778), and it moves the serverSideBlockDefinitions to Redux state of the core/blocks store, and adds some private actions and selectors to that store to work with the bootstrapped block types.

The serverSideBlockDefinitions object is now in state.bootstrappedBlockTypes, and almost all code from unstable__bootstrapServerSideBlockDefinitions has been moved to the boostrappedBlockTypes reducer. It's the code that adds some missing properties and converts property names to camel case. There is a new addBootstrappedBlock private action to add boostrapped blocks, and unstable__bootstrapServerSideBlockDefinitions calls this action in a loop.

The __experimentalRegisterBlockType public action has been renamed to a private action addUnprocessedBlock. It's very unlikely that it's used by any plugin. A wpdirectory.net search shows two matches, but on closer inspection both of them just bundle the @wordpress/blocks package.

Similarly, the __experimentalGetUnprocessedBlockTypes public selector has been renamed to private getUnprocessedBlockTypes.

Now block registration works like this:

  1. Server-bootstrapped block metadata are added with addBootstrappedBlock and stored in state.bootstrappedBlockTypes.
  2. Block settings passed to wp.blocks.registerBlockTypes are stored in state.unprocessedBlockTypes.
  3. There is a processBlockType function (now extracted to a separate module) that takes these two pieces of information ("bootstrapped" metadata and "unprocessed" settings), merges them together, fills default values, and runs the result through registerBlockType filters. The filtered processed result is stored in state.blockTypes. That's where all the user code, like wp.blocks.getBlockTypes(), gets the block types from.
  4. There continues to be the __experimentalReapplyBlockTypeFilters action, called after editor initialization, that reruns the processBlockType job and re-creates state.blockTypes once again from the raw data. That accounts for filters that are registered late.

A good followup for this PR would be a separate PR that renames and stabilizes the unstable__bootstrapServerSideBlockDefinitions and __experimentalReapplyBlockTypeFilters APIs. Both of them are by now established parts of any Gutenberg editor initialization.

@jsnajdr jsnajdr added [Type] Enhancement A suggestion for improvement. [Feature] Blocks Overall functionality of blocks labels Aug 18, 2023
@jsnajdr jsnajdr requested review from gziolo and tyxla August 18, 2023 11:21
@jsnajdr jsnajdr self-assigned this Aug 18, 2023
@github-actions
Copy link

github-actions bot commented Aug 18, 2023

Size Change: +162 B (0%)

Total Size: 1.51 MB

Filename Size Change
build/blocks/index.min.js 51.4 kB +168 B (0%)
build/customize-widgets/index.min.js 12 kB -3 B (0%)
build/edit-post/index.min.js 35.4 kB -1 B (0%)
build/edit-site/index.min.js 91.1 kB -1 B (0%)
build/edit-widgets/index.min.js 16.9 kB -1 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 955 B
build/annotations/index.min.js 2.69 kB
build/api-fetch/index.min.js 2.28 kB
build/autop/index.min.js 2.1 kB
build/blob/index.min.js 451 B
build/block-directory/index.min.js 7.01 kB
build/block-directory/style-rtl.css 1.02 kB
build/block-directory/style.css 1.02 kB
build/block-editor/content-rtl.css 4.25 kB
build/block-editor/content.css 4.24 kB
build/block-editor/default-editor-styles-rtl.css 381 B
build/block-editor/default-editor-styles.css 381 B
build/block-editor/index.min.js 213 kB
build/block-editor/style-rtl.css 15 kB
build/block-editor/style.css 15 kB
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 90 B
build/block-library/blocks/archives/style.css 90 B
build/block-library/blocks/audio/editor-rtl.css 150 B
build/block-library/blocks/audio/editor.css 150 B
build/block-library/blocks/audio/style-rtl.css 122 B
build/block-library/blocks/audio/style.css 122 B
build/block-library/blocks/audio/theme-rtl.css 126 B
build/block-library/blocks/audio/theme.css 126 B
build/block-library/blocks/avatar/editor-rtl.css 116 B
build/block-library/blocks/avatar/editor.css 116 B
build/block-library/blocks/avatar/style-rtl.css 104 B
build/block-library/blocks/avatar/style.css 104 B
build/block-library/blocks/block/editor-rtl.css 305 B
build/block-library/blocks/block/editor.css 305 B
build/block-library/blocks/button/editor-rtl.css 584 B
build/block-library/blocks/button/editor.css 582 B
build/block-library/blocks/button/style-rtl.css 629 B
build/block-library/blocks/button/style.css 628 B
build/block-library/blocks/buttons/editor-rtl.css 337 B
build/block-library/blocks/buttons/editor.css 337 B
build/block-library/blocks/buttons/style-rtl.css 332 B
build/block-library/blocks/buttons/style.css 332 B
build/block-library/blocks/calendar/style-rtl.css 239 B
build/block-library/blocks/calendar/style.css 239 B
build/block-library/blocks/categories/editor-rtl.css 113 B
build/block-library/blocks/categories/editor.css 112 B
build/block-library/blocks/categories/style-rtl.css 124 B
build/block-library/blocks/categories/style.css 124 B
build/block-library/blocks/code/editor-rtl.css 53 B
build/block-library/blocks/code/editor.css 53 B
build/block-library/blocks/code/style-rtl.css 121 B
build/block-library/blocks/code/style.css 121 B
build/block-library/blocks/code/theme-rtl.css 124 B
build/block-library/blocks/code/theme.css 124 B
build/block-library/blocks/columns/editor-rtl.css 108 B
build/block-library/blocks/columns/editor.css 108 B
build/block-library/blocks/columns/style-rtl.css 421 B
build/block-library/blocks/columns/style.css 421 B
build/block-library/blocks/comment-author-avatar/editor-rtl.css 125 B
build/block-library/blocks/comment-author-avatar/editor.css 125 B
build/block-library/blocks/comment-content/style-rtl.css 92 B
build/block-library/blocks/comment-content/style.css 92 B
build/block-library/blocks/comment-template/style-rtl.css 199 B
build/block-library/blocks/comment-template/style.css 198 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css 123 B
build/block-library/blocks/comments-pagination-numbers/editor.css 121 B
build/block-library/blocks/comments-pagination/editor-rtl.css 222 B
build/block-library/blocks/comments-pagination/editor.css 209 B
build/block-library/blocks/comments-pagination/style-rtl.css 235 B
build/block-library/blocks/comments-pagination/style.css 231 B
build/block-library/blocks/comments-title/editor-rtl.css 75 B
build/block-library/blocks/comments-title/editor.css 75 B
build/block-library/blocks/comments/editor-rtl.css 840 B
build/block-library/blocks/comments/editor.css 839 B
build/block-library/blocks/comments/style-rtl.css 637 B
build/block-library/blocks/comments/style.css 636 B
build/block-library/blocks/cover/editor-rtl.css 647 B
build/block-library/blocks/cover/editor.css 650 B
build/block-library/blocks/cover/style-rtl.css 1.63 kB
build/block-library/blocks/cover/style.css 1.62 kB
build/block-library/blocks/details/editor-rtl.css 65 B
build/block-library/blocks/details/editor.css 65 B
build/block-library/blocks/details/style-rtl.css 98 B
build/block-library/blocks/details/style.css 98 B
build/block-library/blocks/embed/editor-rtl.css 293 B
build/block-library/blocks/embed/editor.css 293 B
build/block-library/blocks/embed/style-rtl.css 410 B
build/block-library/blocks/embed/style.css 410 B
build/block-library/blocks/embed/theme-rtl.css 126 B
build/block-library/blocks/embed/theme.css 126 B
build/block-library/blocks/file/editor-rtl.css 316 B
build/block-library/blocks/file/editor.css 316 B
build/block-library/blocks/file/style-rtl.css 280 B
build/block-library/blocks/file/style.css 281 B
build/block-library/blocks/file/view-interactivity.min.js 317 B
build/block-library/blocks/file/view.min.js 375 B
build/block-library/blocks/footnotes/style-rtl.css 201 B
build/block-library/blocks/footnotes/style.css 199 B
build/block-library/blocks/freeform/editor-rtl.css 2.61 kB
build/block-library/blocks/freeform/editor.css 2.61 kB
build/block-library/blocks/gallery/editor-rtl.css 947 B
build/block-library/blocks/gallery/editor.css 952 B
build/block-library/blocks/gallery/style-rtl.css 1.53 kB
build/block-library/blocks/gallery/style.css 1.53 kB
build/block-library/blocks/gallery/theme-rtl.css 108 B
build/block-library/blocks/gallery/theme.css 108 B
build/block-library/blocks/group/editor-rtl.css 654 B
build/block-library/blocks/group/editor.css 654 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 76 B
build/block-library/blocks/heading/style.css 76 B
build/block-library/blocks/html/editor-rtl.css 336 B
build/block-library/blocks/html/editor.css 337 B
build/block-library/blocks/image/editor-rtl.css 834 B
build/block-library/blocks/image/editor.css 833 B
build/block-library/blocks/image/style-rtl.css 1.42 kB
build/block-library/blocks/image/style.css 1.41 kB
build/block-library/blocks/image/theme-rtl.css 126 B
build/block-library/blocks/image/theme.css 126 B
build/block-library/blocks/image/view-interactivity.min.js 1.83 kB
build/block-library/blocks/latest-comments/style-rtl.css 357 B
build/block-library/blocks/latest-comments/style.css 357 B
build/block-library/blocks/latest-posts/editor-rtl.css 213 B
build/block-library/blocks/latest-posts/editor.css 212 B
build/block-library/blocks/latest-posts/style-rtl.css 478 B
build/block-library/blocks/latest-posts/style.css 478 B
build/block-library/blocks/list/style-rtl.css 88 B
build/block-library/blocks/list/style.css 88 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 505 B
build/block-library/blocks/media-text/style.css 503 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 712 B
build/block-library/blocks/navigation-link/editor.css 711 B
build/block-library/blocks/navigation-link/style-rtl.css 115 B
build/block-library/blocks/navigation-link/style.css 115 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 296 B
build/block-library/blocks/navigation-submenu/editor.css 295 B
build/block-library/blocks/navigation/editor-rtl.css 2.26 kB
build/block-library/blocks/navigation/editor.css 2.26 kB
build/block-library/blocks/navigation/style-rtl.css 2.23 kB
build/block-library/blocks/navigation/style.css 2.22 kB
build/block-library/blocks/navigation/view-interactivity.min.js 988 B
build/block-library/blocks/navigation/view-modal.min.js 2.85 kB
build/block-library/blocks/navigation/view.min.js 469 B
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 401 B
build/block-library/blocks/page-list/editor.css 401 B
build/block-library/blocks/page-list/style-rtl.css 175 B
build/block-library/blocks/page-list/style.css 175 B
build/block-library/blocks/paragraph/editor-rtl.css 174 B
build/block-library/blocks/paragraph/editor.css 174 B
build/block-library/blocks/paragraph/style-rtl.css 279 B
build/block-library/blocks/paragraph/style.css 281 B
build/block-library/blocks/post-author/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/editor-rtl.css 96 B
build/block-library/blocks/post-comments-form/editor.css 96 B
build/block-library/blocks/post-comments-form/style-rtl.css 508 B
build/block-library/blocks/post-comments-form/style.css 508 B
build/block-library/blocks/post-date/style-rtl.css 61 B
build/block-library/blocks/post-date/style.css 61 B
build/block-library/blocks/post-excerpt/editor-rtl.css 71 B
build/block-library/blocks/post-excerpt/editor.css 71 B
build/block-library/blocks/post-excerpt/style-rtl.css 141 B
build/block-library/blocks/post-excerpt/style.css 141 B
build/block-library/blocks/post-featured-image/editor-rtl.css 588 B
build/block-library/blocks/post-featured-image/editor.css 586 B
build/block-library/blocks/post-featured-image/style-rtl.css 319 B
build/block-library/blocks/post-featured-image/style.css 319 B
build/block-library/blocks/post-navigation-link/style-rtl.css 153 B
build/block-library/blocks/post-navigation-link/style.css 153 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 314 B
build/block-library/blocks/post-template/style.css 314 B
build/block-library/blocks/post-terms/style-rtl.css 96 B
build/block-library/blocks/post-terms/style.css 96 B
build/block-library/blocks/post-time-to-read/style-rtl.css 69 B
build/block-library/blocks/post-time-to-read/style.css 69 B
build/block-library/blocks/post-title/style-rtl.css 100 B
build/block-library/blocks/post-title/style.css 100 B
build/block-library/blocks/preformatted/style-rtl.css 125 B
build/block-library/blocks/preformatted/style.css 125 B
build/block-library/blocks/pullquote/editor-rtl.css 135 B
build/block-library/blocks/pullquote/editor.css 135 B
build/block-library/blocks/pullquote/style-rtl.css 335 B
build/block-library/blocks/pullquote/style.css 335 B
build/block-library/blocks/pullquote/theme-rtl.css 168 B
build/block-library/blocks/pullquote/theme.css 168 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 221 B
build/block-library/blocks/query-pagination/editor.css 211 B
build/block-library/blocks/query-pagination/style-rtl.css 302 B
build/block-library/blocks/query-pagination/style.css 299 B
build/block-library/blocks/query-title/style-rtl.css 63 B
build/block-library/blocks/query-title/style.css 63 B
build/block-library/blocks/query/editor-rtl.css 450 B
build/block-library/blocks/query/editor.css 449 B
build/block-library/blocks/query/style-rtl.css 370 B
build/block-library/blocks/query/style.css 368 B
build/block-library/blocks/query/view.min.js 559 B
build/block-library/blocks/quote/style-rtl.css 222 B
build/block-library/blocks/quote/style.css 222 B
build/block-library/blocks/quote/theme-rtl.css 223 B
build/block-library/blocks/quote/theme.css 226 B
build/block-library/blocks/read-more/style-rtl.css 132 B
build/block-library/blocks/read-more/style.css 132 B
build/block-library/blocks/rss/editor-rtl.css 149 B
build/block-library/blocks/rss/editor.css 149 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 178 B
build/block-library/blocks/search/editor.css 178 B
build/block-library/blocks/search/style-rtl.css 607 B
build/block-library/blocks/search/style.css 607 B
build/block-library/blocks/search/theme-rtl.css 114 B
build/block-library/blocks/search/theme.css 114 B
build/block-library/blocks/search/view.min.js 631 B
build/block-library/blocks/separator/editor-rtl.css 146 B
build/block-library/blocks/separator/editor.css 146 B
build/block-library/blocks/separator/style-rtl.css 234 B
build/block-library/blocks/separator/style.css 234 B
build/block-library/blocks/separator/theme-rtl.css 194 B
build/block-library/blocks/separator/theme.css 194 B
build/block-library/blocks/shortcode/editor-rtl.css 323 B
build/block-library/blocks/shortcode/editor.css 323 B
build/block-library/blocks/site-logo/editor-rtl.css 754 B
build/block-library/blocks/site-logo/editor.css 754 B
build/block-library/blocks/site-logo/style-rtl.css 204 B
build/block-library/blocks/site-logo/style.css 204 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 116 B
build/block-library/blocks/site-title/editor.css 116 B
build/block-library/blocks/site-title/style-rtl.css 57 B
build/block-library/blocks/site-title/style.css 57 B
build/block-library/blocks/social-link/editor-rtl.css 184 B
build/block-library/blocks/social-link/editor.css 184 B
build/block-library/blocks/social-links/editor-rtl.css 682 B
build/block-library/blocks/social-links/editor.css 681 B
build/block-library/blocks/social-links/style-rtl.css 1.44 kB
build/block-library/blocks/social-links/style.css 1.43 kB
build/block-library/blocks/spacer/editor-rtl.css 348 B
build/block-library/blocks/spacer/editor.css 348 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 432 B
build/block-library/blocks/table/editor.css 432 B
build/block-library/blocks/table/style-rtl.css 639 B
build/block-library/blocks/table/style.css 639 B
build/block-library/blocks/table/theme-rtl.css 146 B
build/block-library/blocks/table/theme.css 146 B
build/block-library/blocks/tag-cloud/style-rtl.css 251 B
build/block-library/blocks/tag-cloud/style.css 253 B
build/block-library/blocks/template-part/editor-rtl.css 403 B
build/block-library/blocks/template-part/editor.css 403 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/term-description/style-rtl.css 111 B
build/block-library/blocks/term-description/style.css 111 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 99 B
build/block-library/blocks/verse/style.css 99 B
build/block-library/blocks/video/editor-rtl.css 552 B
build/block-library/blocks/video/editor.css 555 B
build/block-library/blocks/video/style-rtl.css 185 B
build/block-library/blocks/video/style.css 185 B
build/block-library/blocks/video/theme-rtl.css 126 B
build/block-library/blocks/video/theme.css 126 B
build/block-library/classic-rtl.css 179 B
build/block-library/classic.css 179 B
build/block-library/common-rtl.css 1.1 kB
build/block-library/common.css 1.1 kB
build/block-library/editor-elements-rtl.css 75 B
build/block-library/editor-elements.css 75 B
build/block-library/editor-rtl.css 12.1 kB
build/block-library/editor.css 12.1 kB
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/index.min.js 203 kB
build/block-library/reset-rtl.css 478 B
build/block-library/reset.css 478 B
build/block-library/style-rtl.css 13.8 kB
build/block-library/style.css 13.8 kB
build/block-library/theme-rtl.css 688 B
build/block-library/theme.css 693 B
build/block-serialization-default-parser/index.min.js 1.12 kB
build/block-serialization-spec-parser/index.min.js 2.87 kB
build/commands/index.min.js 15.5 kB
build/commands/style-rtl.css 932 B
build/commands/style.css 929 B
build/components/index.min.js 245 kB
build/components/style-rtl.css 11.8 kB
build/components/style.css 11.8 kB
build/compose/index.min.js 12.1 kB
build/core-commands/index.min.js 2.72 kB
build/core-data/index.min.js 16.8 kB
build/customize-widgets/style-rtl.css 1.46 kB
build/customize-widgets/style.css 1.45 kB
build/data-controls/index.min.js 640 B
build/data/index.min.js 8.38 kB
build/date/index.min.js 17.8 kB
build/deprecated/index.min.js 451 B
build/dom-ready/index.min.js 324 B
build/dom/index.min.js 4.64 kB
build/edit-post/classic-rtl.css 544 B
build/edit-post/classic.css 545 B
build/edit-post/style-rtl.css 7.62 kB
build/edit-post/style.css 7.62 kB
build/edit-site/style-rtl.css 13.2 kB
build/edit-site/style.css 13.2 kB
build/edit-widgets/style-rtl.css 4.52 kB
build/edit-widgets/style.css 4.52 kB
build/editor/index.min.js 45.5 kB
build/editor/style-rtl.css 3.53 kB
build/editor/style.css 3.52 kB
build/element/index.min.js 4.82 kB
build/escape-html/index.min.js 537 B
build/format-library/index.min.js 7.59 kB
build/format-library/style-rtl.css 554 B
build/format-library/style.css 553 B
build/hooks/index.min.js 1.55 kB
build/html-entities/index.min.js 448 B
build/i18n/index.min.js 3.58 kB
build/interactivity/index.min.js 11.2 kB
build/is-shallow-equal/index.min.js 527 B
build/keyboard-shortcuts/index.min.js 1.64 kB
build/keycodes/index.min.js 1.87 kB
build/list-reusable-blocks/index.min.js 2.2 kB
build/list-reusable-blocks/style-rtl.css 836 B
build/list-reusable-blocks/style.css 836 B
build/media-utils/index.min.js 2.9 kB
build/notices/index.min.js 948 B
build/nux/index.min.js 1.99 kB
build/nux/style-rtl.css 735 B
build/nux/style.css 732 B
build/patterns/index.min.js 2.71 kB
build/patterns/style-rtl.css 240 B
build/patterns/style.css 240 B
build/plugins/index.min.js 1.79 kB
build/preferences-persistence/index.min.js 1.84 kB
build/preferences/index.min.js 1.24 kB
build/primitives/index.min.js 943 B
build/priority-queue/index.min.js 1.52 kB
build/private-apis/index.min.js 958 B
build/react-i18n/index.min.js 615 B
build/react-refresh-entry/index.min.js 9.47 kB
build/react-refresh-runtime/index.min.js 7.31 kB
build/redux-routine/index.min.js 2.7 kB
build/reusable-blocks/index.min.js 2.7 kB
build/reusable-blocks/style-rtl.css 243 B
build/reusable-blocks/style.css 243 B
build/rich-text/index.min.js 11 kB
build/router/index.min.js 1.78 kB
build/server-side-render/index.min.js 1.94 kB
build/shortcode/index.min.js 1.39 kB
build/style-engine/index.min.js 1.85 kB
build/sync/index.min.js 53.8 kB
build/token-list/index.min.js 582 B
build/url/index.min.js 3.73 kB
build/vendors/inert-polyfill.min.js 2.48 kB
build/vendors/react-dom.min.js 41.8 kB
build/vendors/react.min.js 4.02 kB
build/viewport/index.min.js 958 B
build/warning/index.min.js 249 B
build/widgets/index.min.js 7.16 kB
build/widgets/style-rtl.css 1.15 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.02 kB

compressed-size-action

@github-actions
Copy link

github-actions bot commented Aug 18, 2023

Flaky tests detected in e6dae48.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6024898479
📝 Reported issues:

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

@jsnajdr haven't done a thorough review just yet, but in the meantime, I noticed that the failing e2e tests have a consistent kind of "The block "undefined" must have a title." error and it persists after retrying. Seems to be something related to the changes suggested here.

@jsnajdr jsnajdr force-pushed the add/boostrapped-blocks-reducer branch from 2d9c9f9 to bee7370 Compare August 25, 2023 07:40
@jsnajdr
Copy link
Member Author

jsnajdr commented Aug 25, 2023

I noticed that the failing e2e tests have a consistent kind of "The block "undefined" must have a title." error

That was a bug in the ADD_UNPROCESSED_BLOCK_TYPE reducer, resulting in block name being registered as undefined. The __experimentalReapplyBlockTypeFilters action then didn't work at all. I pushed a fix for the reducer, and will now work also on a unit test for __experimentalReapplyBlockTypeFilters. It has no coverage at all.

@gziolo
Copy link
Member

gziolo commented Aug 25, 2023

I noticed that the failing e2e tests have a consistent kind of "The block "undefined" must have a title." error

That was a bug in the ADD_UNPROCESSED_BLOCK_TYPE reducer, resulting in block name being registered as undefined. The __experimentalReapplyBlockTypeFilters action then didn't work at all. I pushed a fix for the reducer, and will now work also on a unit test for __experimentalReapplyBlockTypeFilters. It has no coverage at all.

See #34299 for reference. The experimental action had indirect e2e test coverage added, but it would be great to add more tests to increase confidence in this logic. I think the issue is that the name is no longer passed as part of the settings object to the action. Here is the old code that covers it:

https://github.com/WordPress/gutenberg/blob/44a8678975ef85b53199e8161f554479ca1041c0/packages/blocks/src/api/registration.js#L299L315

I believe that the modified action gets raw settings now, while in the past it would standardize it with default values first.

@gziolo
Copy link
Member

gziolo commented Aug 25, 2023

Thank you for opening this PR. It's definitely the area of the codebase that should be improved after we confirmed over time that #34299 resolved the issue with the order of applying filters for the block registration. I need to spend more time processing the changes proposed. I'd like to share some higher-level thoughts for now after I left a previous comment regarding the failing unit test.

A good followup for this PR would be a separate PR that renames and stabilizes the unstable__bootstrapServerSideBlockDefinitions and __experimentalReapplyBlockTypeFilters APIs. Both of them are by now established parts of any Gutenberg editor initialization.

An important note is that
unstable__bootstrapServerSideBlockDefinitions was never meant to be the part of public API. Back then when we had to ship WP 5.0, we didn't have a better way to control what gets exposed to the client for block types registered on the server. Today, we should refactor code to use REST API endpoint for block types instead as outlined in #22812. My only concern would be whether there are any legacy concerns as the post editor is initialized on the server with the context of a specific post type and a post id, so blocks in theory could get modified during registration with filters based on that information. The same issue doesn't apply to the site editor though.

@jsnajdr
Copy link
Member Author

jsnajdr commented Aug 25, 2023

it would be great to add more tests to increase confidence in this logic.

Yes, I added a unit tests that tests the "reapply block filters" logic. Before, it was tested only through e2e tests.

The bug was a simple mistake, I was reading the name value from a value where it wasn't.

I believe that the modified action gets raw settings now, while in the past it would standardize it with default values first.

Yes, I decided to rearrangle the logic in such a way that bootstrappedBlockTypes and unprocessedBlockTypes now store the raw, unmodified data that are passed to registerBlockType( metadata, settings ). It's only the processBlockType that does all the processing, merging the raw data, filling in the defaults etc.

@gziolo
Copy link
Member

gziolo commented Aug 25, 2023

By the way, from my perspective, you can promote __experimentalReapplyBlockTypeFilters to a stable action. It'd be a good idea to keep an experimental name as a deprecated alias as discussed this week during WordPress Community Summit, unless folks will disagree with the notes that will get published soon at https://communitysummit.wordcamp.org/2023/.

@jsnajdr
Copy link
Member Author

jsnajdr commented Aug 25, 2023

Today, we should refactor code to use REST API endpoint for block types

This would mean that an inline script that does:

wp.blocks.unstable__bootstrapServerSideBlockDefinitions({
  'core/paragraph': { title: 'Paragraph' }
});

would be replaced with:

wp.apiFetch.use(wp.apiFetch.createPreloadingMiddleware({
  '/wp/v2/block-types': {
    'body': [ { name: 'core/paragraph', title: 'Paragraph' } ]
  }
});

and the access to the boostrapped block types array would be a select like:

select( coreStore ).getEntityRecord( 'root', 'blockTypes' );

Is that right?

That would work, sure, there's just one caveat: getEntityRecord is async, and to reliably get a non-null value, you are forced to do use the preloading middleware. You can't afford to let the data library make an actual REST request, because then getEntityRecord would be returning null, breaking the entire blocks store. In that way the REST approach is quite fragile.

My only concern would be whether there are any legacy concerns as the post editor is initialized on the server with the context of a specific post type and a post id, so blocks in theory could get modified during registration with filters based on that information.

The preloaded REST data are created in the context of the post editor page, just like the bootstrapServerSideBlockDefinitions data are, so nothing really changes here.

@gziolo
Copy link
Member

gziolo commented Aug 27, 2023

@jsnajdr, you did a great job explaining how that could work. The REST API call would definitely need to be preloaded and further optimized so only essential fields get returned, mirroring what happens today around wp.blocks.unstable__bootstrapServerSideBlockDefinitions call. We can discuss it separately later.

The preloaded REST data are created in the context of the post editor page, just like the bootstrapServerSideBlockDefinitions data are, so nothing really changes here.

Excellent observation. I didn't consider it before. In that case, it should be pretty straightforward refactoring that we can tackle after this PR lands.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Excellent refactoring, @jsnajdr. I did a quite in-depth review and it's looking very close to ready. I noticed one or two potential minor issues, and some room for improvement in terms of how this code was documented in the past (that's on me) to better explain all the ways the block can get registered to satisfy different contexts: client only (covers unit tests and React Native, too) but also client + server.

packages/blocks/src/store/reducer.js Outdated Show resolved Hide resolved
packages/blocks/src/store/reducer.js Outdated Show resolved Hide resolved
[ name ]: getBlockSettingsFromMetadata( blockNameOrMetadata ),
} );
}
const metadata = isObject( blockNameOrMetadata )
Copy link
Member

Choose a reason for hiding this comment

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

Previously, it wouldn't run metadata processing when the first param passed to the function is the string representing the block's name. I believe it needs to remain as it as was because the settings object can provide a different set of keys, and some of them can be already translated like title or description. In effect, only when the blockNameOrMetadata is an object it should call addBootstrappedBlock. Historically, it was always possible to register a block without block.json by providing it's name and settings object that could be used as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

I made this change because it's very useful for async block loading. At some places, like block inserter, my prototype looks at the server bootstrapped data, to avoid fully loading the block:

const { title, parent, supports } = select( blocksStore ).getBootstrappedBlockType( 'core/query-pagination');

But this doesn't work for blocks that are registered only client-side, with no block.json metadata:

registerBlockType( 'core/query-pagination', {
  title: __( 'Pagination' ),
  parent: [ 'core/query' ],
  supports: { inserter: false },
} );

There is no metadata, nothing is added to state.bootstrappedBlockTypes for this block. That's why I'm "reconstructing" the metadata by extracting selected fields from settings.

the settings object can provide a different set of keys, and some of them can be already translated like title or description.

There are two scenarios that can happen here:

  1. the title and description fields were bootstrapped from the server. Then they are localized (the server did that) and the call to addBootstrappedBlock in registerBlockType won't overwrite them. It will do at best the polyfilling of .selectors.
  2. the fields were not bootstrapped from the server, and addBoostrappedBlock will add the data extracted from settings to state.bootstrappedBlockType. The title should be localized, because the JS code will call title: __( 'Pagination' ) if the block is localized at all.

So, in neither of these cases are correct localized data overwritten with incorrent unlocalized data. We're just adding "fallback" bootstrapped metadata in case they don't exist at all.

Copy link
Member

Choose a reason for hiding this comment

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

If it's not using block.json, then it should never have textdomain in the object, so it shouldn't run custom code that applies localization helpers. We are good in that regard.

It still isn't clear to me why it's necessary to fake the block-type bootstrapping in the case where it is registered on the client with:

registerBlockType( 'core/query-pagination', {
  title: __( 'Pagination' ),
  parent: [ 'core/query' ],
  supports: { inserter: false },
} );

There is no metadata, nothing is added to state.bootstrappedBlockTypes for this block. That's why I'm "reconstructing" the metadata by extracting selected fields from settings.

In this case, it should be just fine to bootstrap this block type with an empty object to signal it was also registered. Then, you could do the fallback the other way around and read values directly from the processed version of the block type. You essentially need to fully register the block type anyway to be able to expose it in the inserter.

The only controversial issue is whether to extract "fallback" metadata from a registerBlockType( 'name', { ...settings } ) call. It's useful for the async block loading prototype, but it's also possible to remove this change from this PR and merge it separately, carefully tested.

I guess I don't fully understand the requirements for async loading, so I would say we either pass an empty object for now, or we can skip it altogether so we can discuss it separately and unblock this PR. Otherwise, I think everything is clear and I'm happy to move forward with the refactoring.

Copy link
Member Author

Choose a reason for hiding this comment

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

I reverted to the old boostrapping behavior in e6dae48. Extracting the metadata from settings can be done later, and can be discussed with better context about async block loading.

Having done this, I believe all feedback is addressed 👍

packages/blocks/src/api/test/registration.js Outdated Show resolved Hide resolved

const settings = applyFilters(
'blocks.registerBlockType',
blockType,
Copy link
Member

Choose a reason for hiding this comment

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

Filters might modify the blockType object directly, so a copy { ...blockType } was passed in the past. If we aren't concerned with the modifications, then maybe it would be better to use settings instead of the blockType after this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need to make a copy because blockType is a new object (created from default, bootstrapped and unprocessed settings) on each call.

Before this PR, processBlockType was working directly on an object in Redux state, so a copy was needed.

Still, in both the old and new versions, there is a risk that a filter will do something like:

blockType.supports.inserter = false;

mutating the unprocessed settings object in Redux. We'd need to make a deep copy to avoid that.

Copy link
Member

@gziolo gziolo Aug 30, 2023

Choose a reason for hiding this comment

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

That is a good point about deep copy. Thinking a bit more about all of these, I think the challenge here is that block deprecations have very specific needs. You essentially want to pass the orginal blockType without any modifications/mutations applied with blocks.registerBlockType filter to avoid the situation where the deprecation has its opinions on how to handle the same filter for every deprecation. Although the old implementation is still not resilient to mutation nested objects ...

Well, it doesn't fail any existing unit tests so that might be not an issue at all 🤷🏻

packages/blocks/src/store/private-selectors.js Outdated Show resolved Hide resolved
packages/blocks/src/store/private-selectors.js Outdated Show resolved Hide resolved
packages/blocks/src/store/private-actions.js Outdated Show resolved Hide resolved
@jsnajdr
Copy link
Member Author

jsnajdr commented Aug 28, 2023

The REST API call would definitely need to be preloaded and further optimized so only essential fields get returned, mirroring what happens today

I'm however still skeptical about whether this REST approach is the right way to go. In addition to the fragility caused by REST being async, it also adds a new @wordpress/core-data dependency to the @wordpress/blocks package. And that's undesirable. blocks is a WordPress-agnostic package, while core-data and all its registered entities are all about WordPress REST API.

@jsnajdr
Copy link
Member Author

jsnajdr commented Aug 28, 2023

Thanks @gziolo for a detailed review. I addressed all your feedback. The only controversial issue is whether to extract "fallback" metadata from a registerBlockType( 'name', { ...settings } ) call. It's useful for the async block loading prototype, but it's also possible to remove this change from this PR and merge it separately, carefully tested.

@gziolo
Copy link
Member

gziolo commented Aug 30, 2023

The REST API call would definitely need to be preloaded and further optimized so only essential fields get returned, mirroring what happens today

I'm however still skeptical about whether this REST approach is the right way to go. In addition to the fragility caused by REST being async, it also adds a new @wordpress/core-data dependency to the @wordpress/blocks package. And that's undesirable. blocks is a WordPress-agnostic package, while core-data and all its registered entities are all about WordPress REST API.

We definitely don't want to make the @wordpress/blocks dependant on @wordpress/core-data. The way I was thinking about it boils down to refactoring the current approach in core where we inject the result of get_block_editor_server_block_settings() call with the JS version of reading a similar data from the endpoint:

https://github.com/WordPress/wordpress-develop/blob/e278fe3fcf262d1792a347a460bc004200a2490f/src/wp-admin/edit-form-blocks.php#L107-L111

Anyway, it's a separate issue, and we can discuss it later. The main reason I think it would be valuable is that today, we can only consume core blocks in the mobile app, so that would help to bridge the gap as we would use a single approach to bootstrap blocks registered on the server.


The only controversial issue is whether to extract "fallback" metadata from a registerBlockType( 'name', { ...settings } ) call. It's useful for the async block loading prototype, but it's also possible to remove this change from this PR and merge it separately, carefully tested.

See my comment at #53807 (comment).


It would be useful to mention two changes in the CHANGELOG file:

  • new stable feature: reapplyBlockTypeFilters action
  • deprecations: __experimentalReapplyBlockTypeFilters action
  • should we also mention the removed experiments that are now private APIs?

@jsnajdr jsnajdr force-pushed the add/boostrapped-blocks-reducer branch from 11322b9 to e6dae48 Compare August 30, 2023 12:33

// The `autoInsert` prop is not yet included in the server provided
// definitions and needs to be polyfilled. This can be removed when the
// minimum supported WordPress is >= 6.4.
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this comment, and it implements the suggestion I shared with @ockham in https://github.com/WordPress/gutenberg/pull/52969/files#r1310187507.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Let's go ahead and move forward now that all feedback has been taken care of. Thank you so much for walking me through all the changes proposed. I'm delighted to see all the improvements applied.

@jsnajdr jsnajdr enabled auto-merge (squash) August 30, 2023 13:06
@jsnajdr jsnajdr merged commit bd630b7 into trunk Aug 30, 2023
@jsnajdr jsnajdr deleted the add/boostrapped-blocks-reducer branch August 30, 2023 13:13
@github-actions github-actions bot added this to the Gutenberg 16.6 milestone Aug 30, 2023
@bph bph added [Type] Code Quality Issues or PRs that relate to code quality and removed [Type] Enhancement A suggestion for improvement. labels Aug 30, 2023
@gziolo gziolo mentioned this pull request Sep 5, 2023
3 tasks
@SiobhyB SiobhyB mentioned this pull request Sep 6, 2023
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants