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

Allow classic scripts to depend on modules #8024

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

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Dec 19, 2024

This change proposes allowing a module dependencies to classic scripts.

Classic scripts can add module dependencies like array( 'type' => 'module', 'id' => 'example-module-id' ), where classic script dependencies are simple strings, e.g. 'wp-i18n'.

In order for a classic script to have access to a script module, the script module must appear in the importmap so that it can be accessed by its module identifier from the classic script in a dynamic import.

For a classic script to depend on a module, it must declare that dependency and must be enqueued before the importmap is printed. When the script modules import map is printed, it inspects enqueued scripts to search for module dependencies and includes those dependencies in the import map. Import maps must be printed before any modulepreloads or any script module tags, and only one can be printed (although the specification has changed to allow multiple import maps).

The implementation overrides the WP_Dependencies::add method in WP_Scripts. add is is the main method that handles script registration.
The subclassed implementation is used to partition the provided dependencies array into classic script dependencies and script module dependencies.
The classic script dependencies are passed on to WP_Dependencies::add along with the rest of the arguments. The module dependencies are attached to
the script as extra data module_deps.

When the script modules importmap is printed, it scans through the registered classic scripts for enqueued scripts with module_deps and adds those
modules to the import map.

Script modules in the import map will be available for classic scripts to use via dynamic import().

The dependencies look like this:

wp_register_script_module( '@example/module', /*…*/ );
wp_enqueue_script(
	'example-classic',
	'/classic.js',

	// Dependencies array:
	array(
		// Classic script dependency, no changes.
		'classic-script-dependency',

		// Script module dependency, this form is new.
		array(
			'type' => 'module',
			'id' => '@example/module',
		),
	),
);

Builds on #8009. The implementation here is now independent.

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


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 sirreal changed the title Add module dependency to classic scripts Allow classic scripts to depend on modules Dec 19, 2024
Comment on lines +288 to +297
if (
! $wp_scripts->query( $handle, 'done' ) &&
! $wp_scripts->query( $handle, 'to_do' ) &&
! $wp_scripts->query( $handle, 'enqueued' )
) {
continue;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Is it sufficient to just check enqueued?

Suggested change
if (
! $wp_scripts->query( $handle, 'done' ) &&
! $wp_scripts->query( $handle, 'to_do' ) &&
! $wp_scripts->query( $handle, 'enqueued' )
) {
continue;
}
if ( ! $wp_scripts->query( $handle, 'enqueued' ) ) {
continue;
}

@sirreal sirreal marked this pull request as ready for review December 19, 2024 17:24
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.

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 Dec 19, 2024

@luisherranz @youknowriad @swissspidy @gziolo I'd love to have your review on this approach to allowing classic scripts to depend on script modules.

Comment on lines +287 to +288
if ( $wp_scripts instanceof WP_Scripts ) {
foreach ( $wp_scripts->registered as $dependency ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is similar to what wp_dependencies_unique_hosts does:

foreach ( array( $wp_scripts, $wp_styles ) as $dependencies ) {
if ( $dependencies instanceof WP_Dependencies && ! empty( $dependencies->queue ) ) {
foreach ( $dependencies->queue as $handle ) {

I'm not confident this is the most optimal way to get all classic scripts in the enqueued dependency graph so would appreciate scrutiny or domain knowledge on that.

This allows for script modules to be exposed in the import map
regardless of being a dependency or enqueued.

This will enable scripts to depend on script modules by requesting they
be exposed.
The + operator merges arrays with key overwriting.
array_merge renumbers numeric arrays, the desired behavior in this case.
This works, but it only works as long as scripts are printed before the importmap is printed.
That is undesireable sometimes. An alternate approach may be to invert this scheme and have
script modules inspect enqueued and printed scripts before printing the importmap.
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.

1 participant