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 npm packages without updating browserslist #4699

Conversation

tellthemachines
Copy link
Contributor

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

Alternative to #4698. Updates the @wordpress npm packages without updating browserslist, to avoid the errors from the other PR.


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.

@tellthemachines tellthemachines marked this pull request as ready for review June 26, 2023 06:45
@tellthemachines
Copy link
Contributor Author

tellthemachines commented Jun 26, 2023

PHP unit tests are failing because #4624 hasn't been committed yet.

I'm running into an issue that seems to be related to the framer-motion dependency of @wordpress/components: both the post and site editor crash when a popover is opened. Not entirely sure why this is happening yet.

Update: the culprit seems to be the latest version of framer-motion (10.12.17), installed by default when updating the packages. I tried updating the package to that version in the gutenberg repo and was able to reproduce the crash there.

@youknowriad
Copy link
Contributor

Can we instead of avoiding to update browser list, remove that jsvalidation? Why are we validating built/ignored files anyway, maybe it's just a matter of excluding that js folder from some validation command or else, updating the validation rules to be inline with the latest supported browsers.

@joemcgill
Copy link
Member

Why are we validating built/ignored files anyway...

@youknowriad I was just asking myself this same question when I came across your comment here. From what I can tell this old validation process has become more of a hinderance than a help to our process over the last few releases. Now that it's no longer a maintained library, it may be time to eliminate it completely. Would appreciate you weighing in on the related conversation in Slack.

src/wp-settings.php Outdated Show resolved Hide resolved
@spacedmonkey
Copy link
Member

It seems like is not in this change. WordPress/gutenberg#51866

It is needed now d8409a2 is core.

@ramonjd
Copy link
Member

ramonjd commented Jun 26, 2023

Sync scripts have been updated and committed over at: #4699

@ramonjd
Copy link
Member

ramonjd commented Jun 26, 2023

It seems like is not in this change. WordPress/gutenberg#51866

As far as I know, that PR is marked for backporting to the 16.2RC, which will get backported to core after these packages (in the next day or two).

@ramonjd
Copy link
Member

ramonjd commented Jun 26, 2023

Not sure if it's relevant, but if the sync script is blocked by the deprecated nux package, @ockham says:

I guess one workaround -- to unblock the sync script -- would be to add the wp-6.3 dist tag to the latest existing @wordpress/nux package; but I'm not sure if that's possible for a deprecated npm 🤔

Otherwise, I guess we'll have to remove the dependency before running the sync script, and then add it back afterwards

@tellthemachines
Copy link
Contributor Author

Not sure if it's relevant, but if the sync script is blocked by the deprecated nux package, @ockham says:

The problem isn't the nux package (or that might be a problem, but it's not the main one). It's that some packages were published last week and the versions don't seem to have been updated on trunk, so the version bump from the publishing yesterday is actually a lower version than last weeks for some packages. Afaict the sync task picks up the highest version from npm, so it's picking up the wrong versions for a bunch of packages.

@tellthemachines
Copy link
Contributor Author

Can we instead of avoiding to update browser list, remove that jsvalidation?

I'm fine with doing that, but unless we have any volunteers to get it done in the next few hours, let's leave it until after Beta 1. The priority now is updating the npm packages.

@tellthemachines tellthemachines force-pushed the update/packages-without-browserslist branch from 0ba8f22 to 47ed4e9 Compare June 27, 2023 03:41
@ramonjd
Copy link
Member

ramonjd commented Jun 27, 2023

I've been smoke testing GB 16.1 features and bugfixes.

Things are looking stable! 🙇

I guess we need to commit a few backports to get the unit tests passing (?)

Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

I've got a few notes inline:

  • a few PHP functions have been hard deprecated -- a subset were deprecated in other locations so it will need a little checking
  • reviewed package versions against NPM, all good
  • there are a couple of gutenberg prefixes and suffixes that have made it this far, they'll need to use wp_

I think this looks good for a beta commit though.

"@wordpress/dependency-extraction-webpack-plugin": "4.9.1",
"@wordpress/e2e-test-utils": "9.3.3",
"@wordpress/scripts": "25.3.4",
"@wordpress/babel-preset-default": "7.19.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks expected based on the tags but I am seeing 7.20.0 as the latest release, this version is tagged wp-6.3

"@wordpress/e2e-test-utils": "9.3.3",
"@wordpress/scripts": "25.3.4",
"@wordpress/babel-preset-default": "7.19.1",
"@wordpress/dependency-extraction-webpack-plugin": "4.18.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks expected based on the tags but I am seeing 4.19.0 as the latest release, this version is tagged wp-6.3

"@wordpress/viewport": "5.12.1",
"@wordpress/warning": "2.35.1",
"@wordpress/widgets": "3.12.2",
"@wordpress/wordcount": "3.35.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

All the production wordpress packages are using latest. Lovely 🌻

@@ -14,66 +14,6 @@
* @param bool $is_sub_menu Whether the block is a sub-menu.
* @return array Colors CSS classes and inline styles.
*/
function block_core_navigation_submenu_build_css_colors( $context, $attributes, $is_sub_menu = false ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For follow up: This will need to be properly deprecated to avoid the risk of fatal errors.

Copy link
Member

Choose a reason for hiding this comment

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

How would one deprecate functions that are in block PHP files?

Add them to src/wp-includes/deprecated.php?

I'm not sure we can move them from the block file (at least in Gutenberg) for folks running plugin + WP < 6.3

Copy link
Contributor

Choose a reason for hiding this comment

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

How would one deprecate functions that are in block PHP files?

Add them to src/wp-includes/deprecated.php?

I'm not sure we can move them from the block file (at least in Gutenberg) for folks running plugin + WP < 6.3

@ramonjd I think they'd need to be retained in GB and have a _deprecated_function call added alongside a deprecated annotation. Plugins have a habit of calling the most obscure functions so best to keep it to avoid fatals.

Copy link
Member

@ramonjd ramonjd Jun 30, 2023

Choose a reason for hiding this comment

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

Ah got it. Thanks for the advice @peterwilsoncc

I can get a Gutenberg PR up that does that, e.g.,

... 
 * @deprecated 6.3.0 Use wp_apply_colors_support() instead.
 *
 * @param  array $attributes  Block attributes.
 * @param  bool  $is_sub_menu Whether the block is a sub-menu.
 * @return array Colors CSS classes and inline styles.
 */
function block_core_navigation_submenu_build_css_colors() {
	_deprecated_function( __FUNCTION__, '6.3.0', 'wp_apply_colors_support' );
...

Copy link
Member

Choose a reason for hiding this comment

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

Oh actually, I created a deprecated entry for it:

#4762

*
* @return string Submenu markup with the directives injected.
*/
function gutenberg_block_core_navigation_add_directives_to_submenu( $w, $block_attributes ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Gutenberg prefix (probably for follow up too).

Copy link
Member

Choose a reason for hiding this comment

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

* that parent.
* @return array An array of parsed block data.
*/
function block_core_navigation_parse_blocks_from_menu_items( $menu_items, $menu_items_by_parent_id ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Formal deprecation, Follow up.

🔢 Looks like there are a few to follow up so I'm going to stop re-posting the same things. Applies elsewhere.

*/
function block_core_navigation_parse_blocks_from_menu_items( $menu_items, $menu_items_by_parent_id ) {

_deprecated_function( __FUNCTION__, '6.3.0', 'WP_Navigation_Fallback_Gutenberg::parse_blocks_from_menu_items' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_deprecated_function( __FUNCTION__, '6.3.0', 'WP_Navigation_Fallback_Gutenberg::parse_blocks_from_menu_items' );
_deprecated_function( __FUNCTION__, '6.3.0', 'WP_Navigation_Fallback_Gutenberg::parse_blocks_from_menu_items' );

Gutenberg suffix.

🔢 Appears in a few places.

Copy link
Member

Choose a reason for hiding this comment

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

Removing these in WordPress/gutenberg#51978

/**
* Turns menu item data into a nested array of parsed blocks
*
* @param array $menu_items An array of menu items that represent
Copy link
Contributor

Choose a reason for hiding this comment

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

Follow up: These docblocks will need to a deprecated annotation added.

🔢

Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines 321 to 323
require ABSPATH . WPINC . '/class-wp-block-parser-block.php';
require ABSPATH . WPINC . '/class-wp-block-parser-frame.php';
Copy link
Contributor

Choose a reason for hiding this comment

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

These can remain (they were reordered in the other PR to prevent the fatal by requiring class-wp-block-parser.php first.

@tellthemachines tellthemachines force-pushed the update/packages-without-browserslist branch 2 times, most recently from 5782b95 to 721f3b2 Compare June 27, 2023 08:50
@tellthemachines tellthemachines force-pushed the update/packages-without-browserslist branch from 7139dfd to 21fefa0 Compare June 27, 2023 11:40
if ( ! $navigation_post ) {
$navigation_post = block_core_navigation_maybe_use_classic_menu_fallback();
}
$navigation_post = WP_Navigation_Fallback_Gutenberg::get_fallback();
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be changed to WP_Navigation_Fallback::get_fallback();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah but that has to be changed in the plugin first, these files in core are auto-generated

@ockham
Copy link
Contributor

ockham commented Jun 27, 2023

Committed in https://core.trac.wordpress.org/changeset/56065.

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

Successfully merging this pull request may close these issues.

7 participants