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

Build: Prepare for more Script Modules #7360

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

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Sep 17, 2024

Rework the way that Script Modules are registered.

This is a companion to WordPress/gutenberg#65460 that requires porting to Core. Namely, the block-library changes require registration with their updated script module IDs so that the blocks continue to work correctly. This change is safe to land before the block-library package is updated.

  • "modules" webpack config is renamed "script-modules" aligning with Gutenberg and with the feature name.
  • Script Module registration is handled in one central place.
  • A combined assets file is used for Script Modules registration.
  • Block library Script Module assets that are enqueued on demand are registered in a centralized location. The assets are enqueued on demand. This solves the problem where Gutenberg-specific code is being shipped in Core.
  • The block library Script Module asset Module IDs are renamed to indicate they are view files and align with the naming from Build: Prepare build for more script modules gutenberg#65064, e.g. @wordpress/block-library/query is now @wordpress/block-library/query/view (indicating it is a view file).

A change landed in Gutenberg anticipating this change. It's essential to name the core function wp_default_script_modules accordingly or update Gutenberg:

https://github.com/WordPress/gutenberg/blob/2632234b2bdd6ef8e8b89e7521d370cfa0041764/lib/client-assets.php#L652

This includes the necessary ports from WordPress/gutenberg#65460.

Why?

  • Registering Script Modules in several places is messy and difficult to maintain.
  • Script Modules dependency arrays and versions were manually maintained, which is error prone.
  • Some block library code included Gutenberg-specific functions and constants which is undesirable.

Testing Instructions

Try out this PR. Interactivity script modules (@wordpress/interactivity and @wordpress/interactivity-router) should be served from the script-modules directory.
I've added and reverted a commit that builds with the expected block-library changes soon to be included from Gutenberg. That specific commit can be used to test the block-library for future proof.

Try enabling and disabling SCRIPT_DEBUG. It should control the use of minified or non-minified assets. @wordpress/interactivity should use its debug version of the script with SCRIPT_DEBUG enabled.

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


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

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

sirreal added a commit to WordPress/gutenberg that referenced this pull request Sep 19, 2024
Avoid registering modules twice.

Anticipates changes in WordPress/wordpress-develop#7360
sirreal added a commit to WordPress/gutenberg that referenced this pull request Sep 19, 2024
Avoid registering modules twice.

Anticipates changes in WordPress/wordpress-develop#7360
sirreal added a commit to WordPress/gutenberg that referenced this pull request Sep 20, 2024
Avoid registering modules twice.

Anticipates changes in WordPress/wordpress-develop#7360
sirreal added a commit to WordPress/gutenberg that referenced this pull request Sep 20, 2024
Avoid registering modules twice.

Anticipates changes in WordPress/wordpress-develop#7360
@gziolo
Copy link
Member

gziolo commented Sep 20, 2024

This needs a rebase now after WP packages got synced from Gutenberg.

sirreal added a commit to WordPress/gutenberg that referenced this pull request Sep 20, 2024
Rework how Script Modules are registered in Gutenberg.

Script Module registration is handled in one central place.

A combined assets file is used for Script Modules and registration. This
means that dependencies and versions will be used correctly and kept
up-to-date while avoiding repeated file reads.

Block library Script Module assets that are enqueued on demand _are
registered in a centralized location_. The assets are enqueued on
demand. **This requires a Core change** since the block library PHP
files are synced to Core and also require centralized Script Module
registration (WordPress/wordpress-develop#7360).

This solves a problem where Gutenberg-specific code was being shipped in
Core through block-library.

The block library Script Module asset Module IDs are renamed to indicate
they are view files and align with the naming from #65064:
@wordpress/block-library/query is @wordpress/block-library/query/view
(indicating it is a view file).

---

This is sufficient to change Script Modules to use Gutenberg in a
backwards compatible way:

- `@wordpress/ineractivity` and `@wordpress/interactivity-router` were
  registered on `wp_enqueue_scripts`. That action fires after the
  `wp_default_scripts` used here. Registering an already registered
  Script Module is a no-op. This change registers first.
- The only other Script Modules currently available in Core are from the
  block library. Those have been registered conditionally on use. The ID
  is changed here, so there's little risk of the wrong version being
  used.

There is a Core companion PR that will be necessary to land:
WordPress/wordpress-develop#7360

---

Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
gutenbergplugin pushed a commit to WordPress/gutenberg that referenced this pull request Sep 20, 2024
Rework how Script Modules are registered in Gutenberg.

Script Module registration is handled in one central place.

A combined assets file is used for Script Modules and registration. This
means that dependencies and versions will be used correctly and kept
up-to-date while avoiding repeated file reads.

Block library Script Module assets that are enqueued on demand _are
registered in a centralized location_. The assets are enqueued on
demand. **This requires a Core change** since the block library PHP
files are synced to Core and also require centralized Script Module
registration (WordPress/wordpress-develop#7360).

This solves a problem where Gutenberg-specific code was being shipped in
Core through block-library.

The block library Script Module asset Module IDs are renamed to indicate
they are view files and align with the naming from #65064:
@wordpress/block-library/query is @wordpress/block-library/query/view
(indicating it is a view file).

---

This is sufficient to change Script Modules to use Gutenberg in a
backwards compatible way:

- `@wordpress/ineractivity` and `@wordpress/interactivity-router` were
  registered on `wp_enqueue_scripts`. That action fires after the
  `wp_default_scripts` used here. Registering an already registered
  Script Module is a no-op. This change registers first.
- The only other Script Modules currently available in Core are from the
  block library. Those have been registered conditionally on use. The ID
  is changed here, so there's little risk of the wrong version being
  used.

There is a Core companion PR that will be necessary to land:
WordPress/wordpress-develop#7360

---

Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
@sirreal sirreal force-pushed the build/prepare-more-script-modules branch from 6760209 to 3656c5b Compare September 20, 2024 13:29
@sirreal sirreal marked this pull request as ready for review September 20, 2024 13:29
Copy link

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 jonsurrell, gziolo.

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

@sirreal
Copy link
Member Author

sirreal commented Sep 20, 2024

@gziolo I think CI will pass now, this should be ready for review.

@sirreal
Copy link
Member Author

sirreal commented Sep 20, 2024

There are some test failures, it looks like an easy fix from the newly deprecated function:

Unexpected deprecation notice for WP_Interactivity_API::register_script_modules.

@gziolo
Copy link
Member

gziolo commented Sep 20, 2024

There are some test failures, it looks like an easy fix from the newly deprecated function:

Unexpected deprecation notice for WP_Interactivity_API::register_script_modules.

Tests should no longer run these two deprecated methods.

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.

This is looking great. I’m happy to give it a round of testing next week. This is cool on many levels: better performance due to fewer disk operations, further automated and abstracted logic, great efficiency in using the asset files.

*/
public function add_hooks() {
add_action( 'wp_enqueue_scripts', array( $this, 'register_script_modules' ) );
add_action( 'admin_enqueue_scripts', array( $this, 'register_script_modules' ) );

add_filter( 'script_module_data_@wordpress/interactivity', array( $this, 'filter_script_module_interactivity_data' ) );
Copy link
Member

@gziolo gziolo Sep 20, 2024

Choose a reason for hiding this comment

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

I’m not sure if this should still live here. Could get wired in wp_default_script_modules next to handlers for script modules. No strong preferences, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. My first thought is that this is specific to Interactivity API and it should manage its script module data filters, but it's true that other script modules may have their data filters added in a more generic place.

I'm inclined to leave it now just because there's no obvious reason to move it now. Maybe in time these core script module data filters should all be registered in one place if there are many of them.

I don't feel strongly one way or another.

@sirreal
Copy link
Member Author

sirreal commented Sep 20, 2024

@michalczaplinski @DAreRodz @cbravobernal It would be great to test out the Interactivity API on this PR as well as the interactive blocks in the block library.

There's a reverted commit on this branch that can be re-applied to test with new block-library versions as well. The commit message is "Test with block-library assets from Gutenberg" (currently 404336b).

@sirreal
Copy link
Member Author

sirreal commented Sep 20, 2024

I've started preparing next steps in #7405 to add the a11y script module.

@sirreal sirreal requested a review from gziolo September 20, 2024 21:20
@sirreal sirreal force-pushed the build/prepare-more-script-modules branch from f2715ab to 7836320 Compare September 20, 2024 21:24
peterwilsoncc pushed a commit to peterwilsoncc/gutenberg-build that referenced this pull request Sep 20, 2024
Rework how Script Modules are registered in Gutenberg.

Script Module registration is handled in one central place.

A combined assets file is used for Script Modules and registration. This
means that dependencies and versions will be used correctly and kept
up-to-date while avoiding repeated file reads.

Block library Script Module assets that are enqueued on demand _are
registered in a centralized location_. The assets are enqueued on
demand. **This requires a Core change** since the block library PHP
files are synced to Core and also require centralized Script Module
registration (WordPress/wordpress-develop#7360).

This solves a problem where Gutenberg-specific code was being shipped in
Core through block-library.

The block library Script Module asset Module IDs are renamed to indicate
they are view files and align with the naming from #65064:
@wordpress/block-library/query is @wordpress/block-library/query/view
(indicating it is a view file).

---

This is sufficient to change Script Modules to use Gutenberg in a
backwards compatible way:

- `@wordpress/ineractivity` and `@wordpress/interactivity-router` were
  registered on `wp_enqueue_scripts`. That action fires after the
  `wp_default_scripts` used here. Registering an already registered
  Script Module is a no-op. This change registers first.
- The only other Script Modules currently available in Core are from the
  block library. Those have been registered conditionally on use. The ID
  is changed here, so there's little risk of the wrong version being
  used.

There is a Core companion PR that will be necessary to land:
WordPress/wordpress-develop#7360

---

Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>

Source: WordPress/gutenberg@2632234
peterwilsoncc pushed a commit to peterwilsoncc/gutenberg-build that referenced this pull request Sep 20, 2024
Rework how Script Modules are registered in Gutenberg.

Script Module registration is handled in one central place.

A combined assets file is used for Script Modules and registration. This
means that dependencies and versions will be used correctly and kept
up-to-date while avoiding repeated file reads.

Block library Script Module assets that are enqueued on demand _are
registered in a centralized location_. The assets are enqueued on
demand. **This requires a Core change** since the block library PHP
files are synced to Core and also require centralized Script Module
registration (WordPress/wordpress-develop#7360).

This solves a problem where Gutenberg-specific code was being shipped in
Core through block-library.

The block library Script Module asset Module IDs are renamed to indicate
they are view files and align with the naming from #65064:
@wordpress/block-library/query is @wordpress/block-library/query/view
(indicating it is a view file).

---

This is sufficient to change Script Modules to use Gutenberg in a
backwards compatible way:

- `@wordpress/ineractivity` and `@wordpress/interactivity-router` were
  registered on `wp_enqueue_scripts`. That action fires after the
  `wp_default_scripts` used here. Registering an already registered
  Script Module is a no-op. This change registers first.
- The only other Script Modules currently available in Core are from the
  block library. Those have been registered conditionally on use. The ID
  is changed here, so there's little risk of the wrong version being
  used.

There is a Core companion PR that will be necessary to land:
WordPress/wordpress-develop#7360

---

Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>

Source: WordPress/gutenberg@8488c64
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