-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Build: Prepare for more Script Modules #7360
Conversation
Test using WordPress PlaygroundThe 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
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
Avoid registering modules twice. Anticipates changes in WordPress/wordpress-develop#7360
Avoid registering modules twice. Anticipates changes in WordPress/wordpress-develop#7360
Avoid registering modules twice. Anticipates changes in WordPress/wordpress-develop#7360
Avoid registering modules twice. Anticipates changes in WordPress/wordpress-develop#7360
This needs a rebase now after WP packages got synced from Gutenberg. |
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 <[email protected]> Co-authored-by: gziolo <[email protected]>
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 <[email protected]> Co-authored-by: gziolo <[email protected]>
- Better modules - Parse wp modules from package.json - Combine asset file
6760209
to
3656c5b
Compare
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 Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
@gziolo I think CI will pass now, this should be ready for review. |
src/wp-includes/interactivity-api/class-wp-interactivity-api.php
Outdated
Show resolved
Hide resolved
src/wp-includes/interactivity-api/class-wp-interactivity-api.php
Outdated
Show resolved
Hide resolved
There are some test failures, it looks like an easy fix from the newly deprecated function:
|
Tests should no longer run these two deprecated methods. |
There was a problem hiding this 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' ) ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@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). |
I've started preparing next steps in #7405 to add the a11y script module. |
This pulls the block-library assets from: WordPress/gutenberg@2632234
This reverts commit 404336b.
This script module requires an additional change that will be landed separately before it can be landed. See WordPress/gutenberg#65380.
f2715ab
to
7836320
Compare
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 <[email protected]> Co-authored-by: gziolo <[email protected]> Source: WordPress/gutenberg@2632234
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 <[email protected]> Co-authored-by: gziolo <[email protected]> Source: WordPress/gutenberg@8488c64
Nice work here, thanks. Quick reminder that deadline to commit this backport is 6.7 Beta 1 which is scheduled for 1 October. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* - interactivity/index.min.js => @wordpress/interactivity | ||
* - interactivity/debug.min.js => @wordpress/interactivity/debug | ||
* - block-library/query/view.js => @wordpress/block-library/query/view |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if these paths are the same on Windows. The challenge is that for regular scripts, the path contains only the file name. In this case, it's also folders so let's confirm that there isn't block-library\query\view.js
or something like that. This would mean script-modules-packages.min.php
under version control differs between operating systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe @t-hamano could help with that like in WordPress/gutenberg#65064 (comment).
After npm run build
, here's what's expected at build/wp-includes/js/dist/script-modules
# tree build/wp-includes/js/dist/script-modules
build/wp-includes/js/dist/script-modules
├── block-library
│ ├── file
│ │ ├── view.js
│ │ └── view.min.js
│ ├── image
│ │ ├── view.js
│ │ └── view.min.js
│ ├── navigation
│ │ ├── view.js
│ │ └── view.min.js
│ ├── query
│ │ ├── view.js
│ │ └── view.min.js
│ └── search
│ ├── view.js
│ └── view.min.js
├── interactivity
│ ├── debug.js
│ ├── debug.min.js
│ ├── index.js
│ └── index.min.js
└── interactivity-router
├── index.js
└── index.min.js
9 directories, 16 files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this PR on a Windows host OS. From my understanding, build:dev
does not generate a build
directory, but generates some directories in the src
directory. On the other hand, the build
command generates a complete WordPress in the build
directory.
In both commands, the script-modules-packages.min.php
file contains only forward slashes.
I then replaced the core files in my local WordPress environment on my Windows host OS with the files built in this PR.
The image lightbox works and scripts seem to be enqueued correctly. The navigation block works fine too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you 👍
In both commands, the script-modules-packages.min.php file contains only forward slashes.
Perfect, so that file should be stable across different systems and working correctly 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@t-hamano, thank you so much for testing and confirming it works the same on Windows.
Committed with https://core.trac.wordpress.org/changeset/59083. |
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.
@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?
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 itsdebug
version of the script withSCRIPT_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.