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

Interactivity API: Prevent empty namespace or different namespaces from killing the runtime #61409

Merged
merged 10 commits into from
May 14, 2024

Conversation

cbravobernal
Copy link
Contributor

What?

Similar to #61249, the Interactivity API throws an error in these two cases:

  • If the namespace is an empty string.
  • If the namespace defined on data-wp-interactive is different than the namespace defined in store.js file.

Why?

We don't want that interactive blocks with these errors make the other ones stop working.

How?

Add some checks and try-catchs.

Testing Instructions

Without the PR. Add an interactive block, the data-wp-interactive directive can be diff or an empty string.

render.php

<div
	<?php echo get_block_wrapper_attributes(); ?>
	data-wp-interactive="diff" 
>
    <div <?php echo wp_interactivity_data_wp_context($context) ; ?> >
        <input type="text" data-wp-bind--id="context.id" data-wp-bind--value="context.id"  data-wp-on-document--scroll="actions.log" >
    </div>
    <button data-wp-on--click="actions.log">Not work</button>
</div>
<div data-wp-interactive="create-block">
<button data-wp-on--click="actions.log">Click me</button>
</div>

view.js

/**
 * WordPress dependencies
 */
import { store, getContext } from "@wordpress/interactivity";

store("create-block", {
  actions: {
    log: () => {
      console.log("log 1");
    },
    secondLog: () => {
      console.log("log 2");
    },
  },
});

The second button should not log. Some errors should appear.

With the PR.

The second button should log. Some warnings should appear.

Testing Instructions for Keyboard

Screenshots or screencast

Screenshot 2024-05-06 at 20 01 42

Screenshot 2024-05-06 at 20 02 11

@cbravobernal cbravobernal changed the title Prevent empty namespace or different namespaces from killing the runtime Interactivity API: Prevent empty namespace or different namespaces from killing the runtime May 6, 2024
@cbravobernal cbravobernal added [Type] Bug An existing feature does not function as intended [Packages] Interactivity /packages/interactivity labels May 6, 2024
Copy link

github-actions bot commented May 6, 2024

Size Change: +678 B (+0.04%)

Total Size: 1.75 MB

Filename Size Change
build/block-editor/index.min.js 259 kB -32 B (-0.01%)
build/block-library/blocks/rss/editor-rtl.css 101 B -48 B (-32.21%) 🎉
build/block-library/blocks/rss/editor.css 101 B -48 B (-32.21%) 🎉
build/block-library/blocks/shortcode/editor-rtl.css 286 B -37 B (-11.46%) 👏
build/block-library/blocks/shortcode/editor.css 286 B -37 B (-11.46%) 👏
build/block-library/editor-rtl.css 12.2 kB -29 B (-0.24%)
build/block-library/editor.css 12.2 kB -29 B (-0.24%)
build/block-library/index.min.js 218 kB +118 B (+0.05%)
build/blocks/index.min.js 51.7 kB +40 B (+0.08%)
build/components/index.min.js 222 kB +2.26 kB (+1.03%)
build/components/style-rtl.css 12 kB +39 B (+0.33%)
build/components/style.css 12 kB +34 B (+0.29%)
build/edit-post/index.min.js 14.6 kB +198 B (+1.37%)
build/edit-site/index.min.js 220 kB -2.16 kB (-0.97%)
build/edit-site/style-rtl.css 12.9 kB -49 B (-0.38%)
build/edit-site/style.css 12.9 kB -46 B (-0.36%)
build/editor/index.min.js 85.1 kB +409 B (+0.48%)
build/editor/style-rtl.css 8.34 kB +37 B (+0.45%)
build/editor/style.css 8.35 kB +37 B (+0.45%)
build/interactivity/debug.min.js 16.3 kB +13 B (+0.08%)
build/interactivity/index.min.js 13 kB +10 B (+0.08%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 955 B
build/annotations/index.min.js 2.27 kB
build/api-fetch/index.min.js 2.32 kB
build/autop/index.min.js 2.1 kB
build/blob/index.min.js 578 B
build/block-directory/index.min.js 7.26 kB
build/block-directory/style-rtl.css 1.03 kB
build/block-directory/style.css 1.03 kB
build/block-editor/content-rtl.css 4.57 kB
build/block-editor/content.css 4.57 kB
build/block-editor/default-editor-styles-rtl.css 395 B
build/block-editor/default-editor-styles.css 395 B
build/block-editor/style-rtl.css 15.5 kB
build/block-editor/style.css 15.5 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 133 B
build/block-library/blocks/audio/theme.css 133 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 277 B
build/block-library/blocks/block/editor.css 277 B
build/block-library/blocks/button/editor-rtl.css 415 B
build/block-library/blocks/button/editor.css 414 B
build/block-library/blocks/button/style-rtl.css 627 B
build/block-library/blocks/button/style.css 626 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 671 B
build/block-library/blocks/cover/editor.css 674 B
build/block-library/blocks/cover/style-rtl.css 1.7 kB
build/block-library/blocks/cover/style.css 1.69 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 86 B
build/block-library/blocks/details/style.css 86 B
build/block-library/blocks/embed/editor-rtl.css 312 B
build/block-library/blocks/embed/editor.css 312 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 133 B
build/block-library/blocks/embed/theme.css 133 B
build/block-library/blocks/file/editor-rtl.css 326 B
build/block-library/blocks/file/editor.css 327 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.min.js 324 B
build/block-library/blocks/footnotes/style-rtl.css 201 B
build/block-library/blocks/footnotes/style.css 199 B
build/block-library/blocks/form-input/editor-rtl.css 227 B
build/block-library/blocks/form-input/editor.css 227 B
build/block-library/blocks/form-input/style-rtl.css 343 B
build/block-library/blocks/form-input/style.css 343 B
build/block-library/blocks/form-submission-notification/editor-rtl.css 340 B
build/block-library/blocks/form-submission-notification/editor.css 340 B
build/block-library/blocks/form-submit-button/style-rtl.css 69 B
build/block-library/blocks/form-submit-button/style.css 69 B
build/block-library/blocks/form/view.min.js 471 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 956 B
build/block-library/blocks/gallery/editor.css 960 B
build/block-library/blocks/gallery/style-rtl.css 1.72 kB
build/block-library/blocks/gallery/style.css 1.72 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 394 B
build/block-library/blocks/group/editor.css 394 B
build/block-library/blocks/group/style-rtl.css 103 B
build/block-library/blocks/group/style.css 103 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 189 B
build/block-library/blocks/heading/style.css 189 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 891 B
build/block-library/blocks/image/editor.css 891 B
build/block-library/blocks/image/style-rtl.css 1.6 kB
build/block-library/blocks/image/style.css 1.59 kB
build/block-library/blocks/image/theme-rtl.css 133 B
build/block-library/blocks/image/theme.css 133 B
build/block-library/blocks/image/view.min.js 1.54 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 306 B
build/block-library/blocks/media-text/editor.css 305 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 668 B
build/block-library/blocks/navigation-link/editor.css 669 B
build/block-library/blocks/navigation-link/style-rtl.css 193 B
build/block-library/blocks/navigation-link/style.css 192 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.26 kB
build/block-library/blocks/navigation/style.css 2.25 kB
build/block-library/blocks/navigation/view.min.js 1.03 kB
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 377 B
build/block-library/blocks/page-list/editor.css 377 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 235 B
build/block-library/blocks/paragraph/editor.css 235 B
build/block-library/blocks/paragraph/style-rtl.css 335 B
build/block-library/blocks/paragraph/style.css 335 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-content/editor-rtl.css 74 B
build/block-library/blocks/post-content/editor.css 74 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 734 B
build/block-library/blocks/post-featured-image/editor.css 732 B
build/block-library/blocks/post-featured-image/style-rtl.css 342 B
build/block-library/blocks/post-featured-image/style.css 342 B
build/block-library/blocks/post-navigation-link/style-rtl.css 215 B
build/block-library/blocks/post-navigation-link/style.css 214 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 397 B
build/block-library/blocks/post-template/style.css 396 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 354 B
build/block-library/blocks/pullquote/style.css 353 B
build/block-library/blocks/pullquote/theme-rtl.css 174 B
build/block-library/blocks/pullquote/theme.css 174 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 288 B
build/block-library/blocks/query-pagination/style.css 284 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 486 B
build/block-library/blocks/query/editor.css 486 B
build/block-library/blocks/query/view.min.js 958 B
build/block-library/blocks/quote/style-rtl.css 237 B
build/block-library/blocks/quote/style.css 237 B
build/block-library/blocks/quote/theme-rtl.css 233 B
build/block-library/blocks/quote/theme.css 235 B
build/block-library/blocks/read-more/style-rtl.css 140 B
build/block-library/blocks/read-more/style.css 140 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 184 B
build/block-library/blocks/search/editor.css 184 B
build/block-library/blocks/search/style-rtl.css 690 B
build/block-library/blocks/search/style.css 689 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 478 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 239 B
build/block-library/blocks/separator/style.css 239 B
build/block-library/blocks/separator/theme-rtl.css 194 B
build/block-library/blocks/separator/theme.css 194 B
build/block-library/blocks/site-logo/editor-rtl.css 805 B
build/block-library/blocks/site-logo/editor.css 805 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 324 B
build/block-library/blocks/social-link/editor.css 324 B
build/block-library/blocks/social-links/editor-rtl.css 676 B
build/block-library/blocks/social-links/editor.css 675 B
build/block-library/blocks/social-links/style-rtl.css 1.48 kB
build/block-library/blocks/social-links/style.css 1.48 kB
build/block-library/blocks/spacer/editor-rtl.css 350 B
build/block-library/blocks/spacer/editor.css 350 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 395 B
build/block-library/blocks/table/editor.css 395 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 152 B
build/block-library/blocks/table/theme.css 152 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 393 B
build/block-library/blocks/template-part/editor.css 393 B
build/block-library/blocks/template-part/theme-rtl.css 107 B
build/block-library/blocks/template-part/theme.css 107 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 133 B
build/block-library/blocks/video/theme.css 133 B
build/block-library/classic-rtl.css 179 B
build/block-library/classic.css 179 B
build/block-library/common-rtl.css 1.11 kB
build/block-library/common.css 1.11 kB
build/block-library/editor-elements-rtl.css 75 B
build/block-library/editor-elements.css 75 B
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/reset-rtl.css 472 B
build/block-library/reset.css 472 B
build/block-library/style-rtl.css 14.8 kB
build/block-library/style.css 14.8 kB
build/block-library/theme-rtl.css 707 B
build/block-library/theme.css 713 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.1 kB
build/commands/style-rtl.css 953 B
build/commands/style.css 951 B
build/compose/index.min.js 12.8 kB
build/core-commands/index.min.js 2.81 kB
build/core-data/index.min.js 72.5 kB
build/customize-widgets/index.min.js 10.9 kB
build/customize-widgets/style-rtl.css 1.36 kB
build/customize-widgets/style.css 1.36 kB
build/data-controls/index.min.js 640 B
build/data/index.min.js 9 kB
build/date/index.min.js 17.9 kB
build/deprecated/index.min.js 451 B
build/dom-ready/index.min.js 324 B
build/dom/index.min.js 4.65 kB
build/edit-post/classic-rtl.css 578 B
build/edit-post/classic.css 578 B
build/edit-post/style-rtl.css 2.68 kB
build/edit-post/style.css 2.68 kB
build/edit-widgets/index.min.js 17.5 kB
build/edit-widgets/style-rtl.css 4.18 kB
build/edit-widgets/style.css 4.18 kB
build/element/index.min.js 4.83 kB
build/escape-html/index.min.js 537 B
build/format-library/index.min.js 8.07 kB
build/format-library/style-rtl.css 493 B
build/format-library/style.css 492 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/file.min.js 447 B
build/interactivity/image.min.js 1.67 kB
build/interactivity/navigation.min.js 1.17 kB
build/interactivity/query.min.js 740 B
build/interactivity/router.min.js 2.79 kB
build/interactivity/search.min.js 618 B
build/is-shallow-equal/index.min.js 527 B
build/keyboard-shortcuts/index.min.js 1.3 kB
build/keycodes/index.min.js 1.46 kB
build/list-reusable-blocks/index.min.js 2.11 kB
build/list-reusable-blocks/style-rtl.css 851 B
build/list-reusable-blocks/style.css 851 B
build/media-utils/index.min.js 2.92 kB
build/modules/importmap-polyfill.min.js 12.2 kB
build/notices/index.min.js 948 B
build/nux/index.min.js 1.57 kB
build/nux/style-rtl.css 748 B
build/nux/style.css 744 B
build/patterns/index.min.js 6.45 kB
build/patterns/style-rtl.css 595 B
build/patterns/style.css 595 B
build/plugins/index.min.js 1.8 kB
build/preferences-persistence/index.min.js 2.06 kB
build/preferences/index.min.js 2.85 kB
build/preferences/style-rtl.css 710 B
build/preferences/style.css 712 B
build/primitives/index.min.js 809 B
build/priority-queue/index.min.js 1.52 kB
build/private-apis/index.min.js 1 kB
build/react-i18n/index.min.js 623 B
build/react-refresh-entry/index.min.js 9.47 kB
build/react-refresh-runtime/index.min.js 6.78 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 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 10.1 kB
build/router/index.min.js 1.93 kB
build/server-side-render/index.min.js 1.96 kB
build/shortcode/index.min.js 1.39 kB
build/style-engine/index.min.js 2.02 kB
build/token-list/index.min.js 582 B
build/url/index.min.js 3.74 kB
build/vendors/inert-polyfill.min.js 2.48 kB
build/vendors/react-dom.min.js 41.7 kB
build/vendors/react.min.js 4.03 kB
build/viewport/index.min.js 957 B
build/warning/index.min.js 249 B
build/widgets/index.min.js 7.11 kB
build/widgets/style-rtl.css 1.17 kB
build/widgets/style.css 1.17 kB
build/wordcount/index.min.js 1.02 kB

compressed-size-action

@cbravobernal cbravobernal marked this pull request as ready for review May 6, 2024 19:12
Copy link

github-actions bot commented May 6, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org>
Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
Co-authored-by: luisherranz <luisherranz@git.wordpress.org>
Co-authored-by: DAreRodz <darerodz@git.wordpress.org>
Co-authored-by: michalczaplinski <czapla@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Comment on lines 288 to 290
console.warn(
`The namespace "${ namespace }" defined in "data-wp-interactive" does not match with the store.`
);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this message is accurate. It means we got an error as we tried to follow the path into the store, it doesn't necessarily have anything to do with the namespace.

I think we can go with a message that says that - there was an error when trying to follow the path, being sure to include the path and the namespace.

@cbravobernal cbravobernal force-pushed the fix/prevent-no-namespace-error branch 2 times, most recently from 4fd660f to df4bb57 Compare May 7, 2024 19:47
if ( isDebug ) {
// eslint-disable-next-line no-console
console.warn(
`There was an error when trying to resolve the path "${ path }" in the namespace "${ namespace }".`
Copy link
Member

@luisherranz luisherranz May 8, 2024

Choose a reason for hiding this comment

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

We need to support lazy/dynamically initialized stores. This means that instead of throwing an error, we need to subscribe to the store. This way, when the store is finally loaded, the component will re-render, and everything will work.

I think @DAreRodz was working on it.

To make it as efficient as possible, we need to subscribe as deeply as possible. DeepSignal can subscribe to properties that have not been defined. So, for example, if you are trying to access state.obj.prop and obj is not defined, we should subscribe to state.obj. It shouldn't be complex to do. We just have to let it keep subscribing (which it does automatically) until it throws the error. So basically it's just catching the error.

Copy link
Member

Choose a reason for hiding this comment

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

Is the acc[ key ] access as deep as it can go before erroring sufficient to subscribe?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. It subscribes to the undefined value of the key key. Once that key is replaced with a real value, primitive or object, the computations are invalidated and everything rerenders, resubscribing to the new values (deeper this time if key is an object).

if ( isDebug ) {
// eslint-disable-next-line no-console
console.warn(
`Namespace cannot be an empty string. Error found when trying to use "${ path }"`
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there is a use case for an island that wants to hydrate but doesn't have a namespace, and all its directives use the custom namespace syntax.

Anyway, we can add this and then remove it later if necessary 👍🙂

Copy link
Member

@sirreal sirreal May 8, 2024

Choose a reason for hiding this comment

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

I was worried about that myself and did some testing. This check seems to use the "resolved" namespace, so this warns:

<div data-wp-interactive data-wp-text='state.x'></div>

But this is fine:

<div data-wp-interactive data-wp-text='explicitNS::state.x'></div>

@cbravobernal if we don't have a test like this can we add it? It seems perfectly legitimate to have an interactive region without its own namespace.

Copy link
Contributor

@DAreRodz DAreRodz May 8, 2024

Choose a reason for hiding this comment

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

I think these won't warn (see #61409 (comment)):

<div data-wp-interactive='null' data-wp-text='state.x'></div>
<div data-wp-interactive='{}' data-wp-text='state.x'></div>

Copy link
Member

Choose a reason for hiding this comment

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

Good call @DAreRodz - I think we should revisit this section to fix some of that - we JSON.parse attribute values too aggressively:

let [ ns, value ] = nsPathRegExp
.exec( attributes[ i ].value )
?.slice( 1 ) ?? [ null, attributes[ i ].value ];
try {
value = JSON.parse( value );
} catch ( e ) {}
if ( n === islandAttr ) {
island = true;
namespaces.push(
typeof value === 'string'
? value
: value?.namespace ?? null
);
} else {
directives.push( [ n, ns, value ] );
}

Maybe something like this (only for the interactive directive):

  • If the value is falsy => null ns
  • If the value starts with { (I don't think namespaces are allowed to start with this?)
    • Try to parse and get the namespace property
    • otherwise null
  • Otherwise, the namespace is the string - should we check that it's a valid namespace??

I don't think that any other directives should ever be JSON parsed.

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 don't think that any other directives should ever be JSON parsed.

What about data-wp-context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<div data-wp-interactive='null' data-wp-text='state.x'></div>
<div data-wp-interactive='{}' data-wp-text='state.x'></div>

Now they should.

Maybe something like this (only for the interactive directive)....

Should we do this in a follow-up PR?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, let's leave those refactors for another PR.

What about data-wp-context?

🙃

@sirreal sirreal self-requested a review May 8, 2024 08:18
@@ -260,18 +261,35 @@ export const directive = (

// Resolve the path to some property of the store object.
const resolve = ( path, namespace ) => {
if ( namespace === '' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering why this strict check, and it looks like when data-wp-interactive is empty, we don't add null to the namespace stack but empty strings.

Just follow this logic, considering that value is an empty string:

try {
value = JSON.parse( value );
} catch ( e ) {}
if ( n === islandAttr ) {
island = true;
namespaces.push(
typeof value === 'string'
? value
: value?.namespace ?? null
);

As the JSON.parse() execution fails, value is still an empty string, the type is string, so the final namespace we push is ''.

However, if the runtime has not processed any data-wp-interactive element, or the value is null or an empty object, the namespace can actually be null. In those cases, the check could fail. 😬

I guess we could use null everywhere to make it consistent when no namespace is defined, right? 🤔
And also we can simply check if namespace is a falsy value or not.

Suggested change
if ( namespace === '' ) {
if ( ! namespace ) {

cc: @luisherranz, @sirreal

Copy link
Member

Choose a reason for hiding this comment

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

That seems good 👍

Copy link
Member

Choose a reason for hiding this comment

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

If the namespace is falsy, it should not do anything, which means keeping the current namespace. But since we are popping the namespace afterward, the current namespace must be added again, whether null or a valid namespace.

Does that make sense?

By the way, can anyone check how the logic works on the server? It should be consistent in both places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, can anyone check how the logic works on the server? It should be consistent in both places.

I'll do it.

@cbravobernal
Copy link
Contributor Author

I refactored it a little and added some tests. Feel free to re-review.

@cbravobernal cbravobernal requested a review from DAreRodz May 9, 2024 16:38
@cbravobernal cbravobernal force-pushed the fix/prevent-no-namespace-error branch from 3df11e5 to 414e15b Compare May 10, 2024 14:05

export const warn = ( message ) => {
// @ts-expect-error
if ( typeof SCRIPT_DEBUG !== 'undefined' && SCRIPT_DEBUG === true ) {
Copy link
Member

Choose a reason for hiding this comment

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

Flagging as something I'll need to update in #61486

return path.split( '.' ).reduce( ( acc, key ) => acc[ key ], current );
} catch ( e ) {
warn(
`The path "${ path }" could not be resolved in the "${ namespace }" store.`
Copy link
Member

Choose a reason for hiding this comment

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

Loading a store after the initial hydration is legit; I don't think we should add a warning that we must remove later. It's going to cause confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in f587e3d

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, Carlos.

Copy link
Contributor

@michalczaplinski michalczaplinski left a comment

Choose a reason for hiding this comment

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

LGTM sans the minor typo I mentioned

packages/interactivity/src/utils/warn.ts Show resolved Hide resolved
packages/interactivity/src/hooks.tsx Outdated Show resolved Hide resolved
@cbravobernal cbravobernal enabled auto-merge (squash) May 14, 2024 12:56
Copy link

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ packages/e2e-tests/plugins/interactive-blocks/namespace/render.php
❔ packages/e2e-tests/plugins/interactive-blocks/namespace/view.asset.php

@cbravobernal cbravobernal merged commit c70ea83 into trunk May 14, 2024
62 checks passed
@cbravobernal cbravobernal deleted the fix/prevent-no-namespace-error branch May 14, 2024 13:51
@github-actions github-actions bot added this to the Gutenberg 18.4 milestone May 14, 2024
@ajlende ajlende added the [Feature] Interactivity API API to add frontend interactivity to blocks. label May 15, 2024
@gziolo
Copy link
Member

gziolo commented May 17, 2024

Flagging that all warning messages end up being included in the production bundle when using the warn util. They are never printed on production because the body of the helper method goes through the tree-shaking step.

Screenshot 2024-05-17 at 11 47 34 Screenshot 2024-05-17 at 11 49 21 Screenshot 2024-05-17 at 11 48 32

@wordpress/warning package uses this Babel plugin: https://github.com/WordPress/gutenberg/blob/trunk/packages/warning/babel-plugin.js to ensure that all usage also gets automatically wrapped with the check against SCRIPT_DEBUG, similar to how it is implemented in the util.

@sirreal
Copy link
Member

sirreal commented May 17, 2024

To add some context, the babel plugin mentioned works because it's going to transform the warn() calls into something else.

What we have here is like this:

if ( problem ) {
warn( 'oh no' );
}

function warn( msg ) {
  if (SCRIPT_DEBUG) { /*…*/ };
}

After compilation (replacement and optimization) we get this:

if ( problem ) {
warn( 'oh no' );
}

function warn( msg ) {}

So the message and the function call are still there, which is what we'd hoped to eliminate in optimization.

The babel plugin works by replacing the warn call so that the constant condition is on the outside:

if (SCRIPT_DEBUG) {
  warn( 'oh no' );
};
// optimized - completely eliminated

That's why the function call is nice for reducing duplication, but is annoying to optimize away (requiring some more-or-less heavy-handed transformation).

One idea I was playing with was terser annotations. If we always annotate the warn function to be inlined, it might be eliminated. Or it could be conditionally annotated as pure in production builds and again it should be eliminated.

@sirreal
Copy link
Member

sirreal commented May 17, 2024

I created #61765 to track this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Interactivity API API to add frontend interactivity to blocks. [Packages] Interactivity /packages/interactivity [Type] Bug An existing feature does not function as intended
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants