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

Update packages for 6.8 Beta 1 #8224

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

Mamaduka
Copy link
Member

@Mamaduka Mamaduka commented Jan 30, 2025

A list of known issues:

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


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

github-actions bot commented Jan 30, 2025

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.

Core Committers: Use this line as a base for the props when committing in SVN:

Props mamaduka, joemcgill.

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

@Mamaduka Mamaduka requested review from desrosj and joemcgill January 30, 2025 15:34
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 @Mamaduka! Doing some initial testing to ensure nothing obvious is broken, but no initial big red flags from me.

I see that this sync is including several new __experimental… settings, like __experimentalBorder and continues the use of existing __experimentalDefaultControls settings.

I've not been involved in these syncs in the past, but was under the impression that we were attempting to move away from shipping these kinds of features under the __experimental… prefix. Am I mistaken? Regardless, I don't think that's a blocker for this sync, since those settings could be stabilized to remove the prefix, or removed entirely prior to shipping a final release. I mainly want to make sure I understand your expectations for how this is meant to work.

Comment on lines +96 to +103
'@wordpress/dataviews/wp',
'@wordpress/icons',
'@wordpress/interface',
'@wordpress/interactivity',
'@wordpress/sync',
'@wordpress/undo-manager',
'@wordpress/upload-media',
'@wordpress/fields',
Copy link
Member

Choose a reason for hiding this comment

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

Noting the new packages being bundled and their purpose in the commit message would probably be a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

Noted! Only @wordpress/upload-media is the new package here; it looks like we missed synchronization of this config in past releases.

I'll double-check new values here before this is committed.

@@ -56,6 +56,7 @@
remove_action( 'init', 'register_block_core_query_pagination_numbers' );
remove_action( 'init', 'register_block_core_query_pagination_previous' );
remove_action( 'init', 'register_block_core_query_title' );
remove_action( 'init', 'register_block_core_query_total' );
Copy link
Member

Choose a reason for hiding this comment

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

👍🏻 New action being added to register the new core/query-total block.

@Mamaduka
Copy link
Member Author

I've not been involved in these syncs in the past, but was under the impression that we were attempting to move away from shipping these kinds of features under the __experimental… prefix.

@joemcgill, that only applies to JS API. The block.json is a configuration file that can't have private APIs, hence the __experimental prefixes. They're and will be backward compatible.

@joemcgill
Copy link
Member

Thanks for clarifying! Do we have any documentation or best practice for why and when to use those prefixes for block.json features? If they are intended to be backward compatible, should we be looking to remove these prefixes?

@Mamaduka
Copy link
Member Author

These aren't new configuration values; they just got enabled for those blocks. When those features are stable, we usually create iteration issues like #64314 and stabilize the keys.

I don't recall any official docs, but it is probably worth documenting.

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.

I was able to reproduce the error with the @wordpress/fields package, noted in the PR description.

I'm also seeing the following, which makes the site editor unusable currently.

Uncaught TypeError: Cannot read properties of undefined (reading 'initializeEditor')
    at HTMLDocument.<anonymous> (site-editor.php:2369:16)

I'm adding request changes until we're confident in these being resolved in a way that doesn't totally block the site editor from being usable in trunk

To reproduce:

  1. Check out this PR
  2. Run npm i && npm run build:dev
  3. Visit the site editor with no other plugins activated

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.

2 participants