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

WIP: Update npm packages to latest versions for 6.4 Beta 1 #5262

Closed
wants to merge 77 commits into from

Conversation

mikachan
Copy link
Member

@mikachan mikachan commented Sep 20, 2023

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

This PR includes changes from #5235 & #5195.

I'm currently seeing the following error:

Running "verify:source-maps" task
Warning: The build/wp-includes/js/dist/sync.js file must not contain a sourceMappingURL. Use --force to continue.

I'm not sure if there are any other missing changes that should be included in this PR - perhaps that's all that's missing for the sync script to run successfully.


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.

@luisherranz
Copy link
Member

This is a potential fix for the Warning: The build/wp-includes/js/dist/sync.js file must not contain a sourceMappingURL error until you reach @youknowriad for confirmation: luisherranz@e38b785

@luisherranz
Copy link
Member

There is a mismatch with the type of files that are enqueued in the frontend. When you run npm run build:dev or npm run dev, the file enqueued for the Interactivity API runtime is the development version (interactivity.js), which is correct, but the files enqueued for the blocks are the production version (view.min.js instead of view.js).

It looks like in trunk the files enqueued for the blocks are also the production version, so it's not something we introduced. But we need to fix it. I'll try to locate where those enqueues happen.

On the other hand, I've manually copied the files and they work fine when they match (both are development or both are production), so the new Webpack configuration seems to have worked well and I don't think we'll need to make any further adjustments.

@luisherranz
Copy link
Member

It looks like in trunk the files enqueued for the blocks are also the production version, so it's not something we introduced. But we need to fix it. I'll try to locate where those enqueues happen.

It looks like those files are hardcoded in the block.json:

"viewScript": "file:./view.min.js",

So for the moment, instead of honoring the development/production mode, let's always enqueue the production version of the Interactivity API runtime to match what the blocks are doing.

@mikachan, you can cherry-pick this commit to fix this: luisherranz@facb976

package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Thanks for getting this started! Looks like we'll need to make a few more changes in Gutenberg and republish the packages. I'm not quite sure why, but removing the sync package and rebuilding is causing neither editor to load in my local env. Can anyone reproduce this?

It looks like those files are hardcoded in the block.json:

These changes will also have to be made in Gutenberg.

package.json Outdated Show resolved Hide resolved
src/wp-includes/blocks/file.php Show resolved Hide resolved
src/wp-includes/blocks/image.php Outdated Show resolved Hide resolved
@luisherranz
Copy link
Member

These two files can be removed now because the view-modal.js file doesn't exist anymore:

  • src/wp-includes/blocks/navigation/view-modal.asset.php
  • src/wp-includes/blocks/navigation/view-modal.min.asset.php

Apart from that, the new Webpack configuration is adding two unnecessary files to the js/dist folder:

  • src/wp-includes/js/dist/interactivity.asset.php
  • src/wp-includes/js/dist/interactivity.min.asset.php

I guess it'd be better to delete them. We can do so using this: luisherranz@9ad3846

@mikachan
Copy link
Member Author

Thanks everyone for all the help here.

I think I've cherry-picked all the changes you mentioned, @luisherranz. The sync script is now running successfully 🎉, but I know we need a fresh GB 16.7 RC as well. I can cherry-pick the new GB PRs and trigger a build as soon as all the needed changes are merged.

mikachan and others added 3 commits September 21, 2023 10:15
# Conflicts:
#	package-lock.json
#	package.json
#	src/wp-includes/assets/script-loader-packages.min.php
#	src/wp-includes/blocks/file/view.asset.php
#	src/wp-includes/blocks/file/view.min.asset.php
#	src/wp-includes/blocks/navigation/view.asset.php
#	src/wp-includes/blocks/navigation/view.min.asset.php
#	src/wp-includes/blocks/search/view.asset.php
Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

A couple things still need to be addressed; I'm mostly concerned about the sync package. We should remove it, which will require a few more changes in Gutenberg. Can we hold off on republishing the packages until this is sorted out?

Gruntfile.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/wp-includes/blocks/image.php Outdated Show resolved Hide resolved
…rds in `phpunit/tests/media.php`."

This reverts commit 89f228b.
…ed for loading optimization attributes."

This reverts commit 99a2b31.
…add `decoding="async"` to images."

This reverts commit 6bbce3a.
…cript_tag()`/`wp_print_inline_script_tag()` helper functions to output scripts on the frontend and login screen."

This reverts commit a6bf745.
…lls to `wp_add_inline_style`."

This reverts commit 1e1ed26.
…se writes in `update_option()`."

This reverts commit 75c3843.
# Conflicts:
#	src/wp-includes/assets/script-loader-packages.min.php
#	src/wp-includes/blocks/file/view.min.asset.php
#	src/wp-includes/blocks/search/view.min.asset.php
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

One more non-blocking recommendation.

Given the beta timeline, I don't want to hold up this PR, but it would be great to consider this change prior to the next beta.

Comment on lines +282 to +284
// Add the private version of the Interactivity API manually.
$scripts->add( 'wp-interactivity', '/wp-includes/js/dist/interactivity.min.js' );
did_action( 'init' ) && $scripts->add_data( 'wp-interactivity', 'strategy', 'defer' );
Copy link
Member

Choose a reason for hiding this comment

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

Per my response in #5262 (comment), given we want to avoid exposing this API too much until it's stable, how about we don't register it unconditionally?

We could introduce a function like this:

function _wp_maybe_enqueue_interactivity() {
	if ( wp_script_is( 'wp-interactivity' ) ) {
		return;
	}
	wp_enqueue_script(
		'wp-interactivity',
		'/wp-includes/js/dist/interactivity.min.js',
		array(),
		false,
		array( 'strategy' => 'defer' )
	);
}

That function could then be called in all the blocks where it is needed, and with such an approach the use of the wp-interactivity script would be more obviously discouraged.

Copy link
Member Author

Choose a reason for hiding this comment

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

cc. @luisherranz for your thoughts here.

Copy link
Member

@luisherranz luisherranz Sep 28, 2023

Choose a reason for hiding this comment

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

Thanks for the ping, @mikachan 🙂


given we want to avoid exposing this API too much until it's stable, how about we don't register it unconditionally?

@felixarntz:

  1. If your worries are about third-party scripts accessing the exports that are inside of that JavaScript file:

    That file is generated by Webpack as an independent chunk. Only the entry points generated in the same webpack config have the correct auto-generated ID to access those exports.

    I'm happy to explore other additional measures. For example, I also tested a random ID that changes each time Webpack runs, although it didn't feel necessary to me as accessing a private Webpack generated chunk is already very hacky and there's no guarantee that the auto-generated ID won't change either.

  2. If your worries are about having a script that sometimes is registered unnecessarily and you would prefer to make sure that it's only registered if it's finally enqueued: sure, we can try to add logic to check for that 🙂

Just let me know and I'll take care of it as soon as possible.

Copy link
Member

Choose a reason for hiding this comment

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

@hellofromtonya
Copy link
Contributor

Closing as https://core.trac.wordpress.org/changeset/56710 has committed the package updates.

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.