Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 69 additions & 22 deletions src/wp-includes/class-wp-script-modules.php
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,18 @@ class WP_Script_Modules {
*/
private $modules_with_missing_dependencies = array();

/**
* List of handles for classic scripts encountered which have missing script module dependencies.
*
* A handle is added to this list when it is discovered to have script module dependencies that are not
* registered. At this time, a warning is emitted with {@see _doing_it_wrong()}. The handle is then added to this
* list, so that duplicate warnings don't occur.
*
* @since 7.2.0
* @var string[]
*/
private $classic_scripts_with_missing_module_dependencies = array();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Naming nit: let's call these just "scripts", not "classic scripts". Everything else in the codebase calls them "scripts". The WP_Scripts class, functions like wp_register_script, actions like admin_print_scripts, etc.


/**
* Registers the script module if no script module with that script module
* identifier has already been registered.
Expand Down Expand Up @@ -367,7 +379,8 @@ public function set_translations( string $id, string $domain = 'default', string
}

/**
* Prints translations for all enqueued script modules.
* Prints translations for all enqueued script modules and for the script
* modules that classic scripts depend on.
*
* Outputs inline `<script>` tags that call `wp.i18n.setLocaleData()` with
* the translated strings for each script module. This must run before
Expand All @@ -379,10 +392,23 @@ public function set_translations( string $id, string $domain = 'default', string
* {@see WP_Script_Modules::set_translations()}.
*
* @since 7.0.0
* @since 7.2.0 Also prints the translations of the script modules that classic scripts depend on
* ('module_dependencies'), and of their dependencies.
*/
public function print_script_module_translations(): void {
// Collect all module IDs that will be on the page (enqueued + their dependencies).
$module_ids = $this->get_sorted_dependencies( $this->queue );
/*
* Collect the IDs of the script modules that can load on the page: the enqueued ones, the ones that classic
* scripts depend on, and the dependencies of both.
*/
$classic_script_module_dependencies = $this->get_classic_script_module_dependencies();

$module_ids = array_unique(
array_merge(
$this->get_sorted_dependencies( $this->queue ),
$classic_script_module_dependencies,
array_keys( $this->get_dependencies( $classic_script_module_dependencies ) )
)
);
Comment on lines +399 to +411

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

print_script_module_data() already builds "every module that can load" from the queue plus the import map keys:

public function print_script_module_data(): void {
$modules = array();
foreach ( array_unique( $this->queue ) as $id ) {
if ( '@wordpress/a11y' === $id ) {
$this->a11y_available = true;
}
$modules[ $id ] = true;
}
foreach ( array_keys( $this->get_import_map()['imports'] ) as $id ) {
if ( '@wordpress/a11y' === $id ) {
$this->a11y_available = true;
}
$modules[ $id ] = true;
}

The same works here and gives the same set: the import map is the classic script module dependencies plus get_dependencies() of the queue and of those, so only the queue itself needs adding. The one difference is a module whose filtered src is '', which cannot load anyway. This keeps the import map, data and translations in lockstep by construction, and the get_classic_script_module_dependencies() extraction is no longer needed (the once-per-handle notice guard still is, since get_import_map() would then run three times).

Suggested change
/*
* Collect the IDs of the script modules that can load on the page: the enqueued ones, the ones that classic
* scripts depend on, and the dependencies of both.
*/
$classic_script_module_dependencies = $this->get_classic_script_module_dependencies();
$module_ids = array_unique(
array_merge(
$this->get_sorted_dependencies( $this->queue ),
$classic_script_module_dependencies,
array_keys( $this->get_dependencies( $classic_script_module_dependencies ) )
)
);
/*
* Collect the IDs of the script modules that can load on the page: the enqueued ones and their dependencies,
* and the ones in the import map, which includes the script modules that classic scripts depend on.
*/
$module_ids = array_unique(
array_merge(
$this->get_sorted_dependencies( $this->queue ),
array_keys( $this->get_import_map()['imports'] )
)
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, I think the biggest issue after we have this PR is that there are two loops over all reachable modules, print_script_module_translations and print_script_module_data, which should loop exactly the same way, but they don't. get_sorted_dependencies is used inconsistently: one loop has, the other doesn't. And the classic dependencies are not sorted at all, although they should be.

get_import_data loops differently, but not differently enough to have a completely custom code. It subtracts the already queued modules and it also skips modules with src=''. The additional src='' check, maybe it should be a validity check that applies to all modules? I don't think we support inline modules.

Unifying the translations and data behaviors in important, even a blocker, get_import_data can remain custom.


$set_locale_data_js_function = <<<'JS'
( domain, translations ) => {
Expand Down Expand Up @@ -636,17 +662,47 @@ public function print_import_map() {
* @since 6.5.0
* @since 7.0.0 Script module dependencies ('module_dependencies') of classic scripts are now included.
*
* @global WP_Scripts $wp_scripts
*
* @return array<string, array<string, string>> Array with an `imports` key mapping to an array of script module
* identifiers and their respective URLs, including the version query.
*/
private function get_import_map(): array {
global $wp_scripts;

$imports = array();

// Identify script modules that are dependencies of classic scripts.
$classic_script_module_dependencies = $this->get_classic_script_module_dependencies();

// Note: the script modules in $this->queue are not included in the importmap because they get printed as scripts.
$ids = array_unique(
array_merge(
$classic_script_module_dependencies,
array_keys( $this->get_dependencies( array_merge( $this->queue, $classic_script_module_dependencies ) ) )
)
);
foreach ( $ids as $id ) {
$src = $this->get_src( $id );
if ( '' !== $src ) {
$imports[ $id ] = $src;
}
}
return array( 'imports' => $imports );
}

/**
* Gets the IDs of the script modules that classic scripts depend on.
*
* Walks the enqueued, to-do and done classic scripts and their dependencies, and collects the registered
* script modules in their 'module_dependencies' data. Script modules that are not registered trigger a
* {@see _doing_it_wrong()} notice, once per classic script.
*
* @since 7.2.0
*
* @global WP_Scripts $wp_scripts
*
* @return string[] Script module IDs.
*/
private function get_classic_script_module_dependencies(): array {
global $wp_scripts;

$classic_script_module_dependencies = array();
if ( $wp_scripts instanceof WP_Scripts ) {
$handles = array_merge(
Expand Down Expand Up @@ -684,7 +740,10 @@ private function get_import_map(): array {
}
}

if ( count( $missing_module_dependencies ) > 0 ) {
if (
count( $missing_module_dependencies ) > 0 &&
! in_array( $handle, $this->classic_scripts_with_missing_module_dependencies, true )
) {
_doing_it_wrong(
'WP_Scripts::add_data',
sprintf(
Expand All @@ -696,6 +755,7 @@ private function get_import_map(): array {
),
'7.0.0'
);
$this->classic_scripts_with_missing_module_dependencies[] = $handle;
}
}

Expand All @@ -707,20 +767,7 @@ private function get_import_map(): array {
}
}

// Note: the script modules in $this->queue are not included in the importmap because they get printed as scripts.
$ids = array_unique(
array_merge(
$classic_script_module_dependencies,
array_keys( $this->get_dependencies( array_merge( $this->queue, $classic_script_module_dependencies ) ) )
)
);
foreach ( $ids as $id ) {
$src = $this->get_src( $id );
if ( '' !== $src ) {
$imports[ $id ] = $src;
}
}
return array( 'imports' => $imports );
return $classic_script_module_dependencies;
}

/**
Expand Down
240 changes: 240 additions & 0 deletions tests/phpunit/tests/script-modules/wpScriptModules.php
Original file line number Diff line number Diff line change
Expand Up @@ -2924,4 +2924,244 @@ static function ( $translations, $file, $handle, $domain ) use ( &$seen_domain )
$this->assertSame( 'my-plugin', $seen_domain, 'load_script_module_textdomain() should be called with the overridden domain.' );
$this->assertStringContainsString( 'Hola', $output, 'Output should contain the translated string loaded under the overridden domain.' );
}

/**
* Provides translations for the given script modules through the `pre_load_script_translations` filter.
*
* @param string[] $ids Script module IDs.
*/
private function add_script_module_translations( array $ids ): void {
add_filter(
'pre_load_script_translations',
static function ( $translations, $file, $handle ) use ( $ids ) {
if ( ! in_array( $handle, $ids, true ) ) {
return $translations;
}
return wp_json_encode(
array(
'locale_data' => array(
'messages' => array(
'' => array(
'domain' => 'messages',
'lang' => 'es',
),
"Hello from $handle" => array( "Hola desde $handle" ),
),
),
)
);
},
10,
3
);
}

/**
* Gets the IDs of the script modules whose translations are printed in the given markup.
*
* @param string $markup Markup printed by print_script_module_translations().
* @return string[] Script module IDs, in the order their translations are printed.
*/
private function get_script_module_translation_ids( string $markup ): array {
$prefix = 'wp-script-module-translation-data-';
$ids = array();
$processor = new WP_HTML_Tag_Processor( $markup );
while ( $processor->next_tag( 'SCRIPT' ) ) {
$id = $processor->get_attribute( 'id' );
if ( is_string( $id ) && str_starts_with( $id, $prefix ) ) {
$ids[] = substr( $id, strlen( $prefix ) );
}
}
return $ids;
}

/**
* Tests that print_script_module_translations() outputs translations for a script module that only a classic
* script depends on, and for the static and dynamic dependencies of that script module.
*
* @dataProvider data_classic_script_module_dependencies
*
* @covers WP_Script_Modules::print_script_module_translations
* @covers WP_Script_Modules::get_classic_script_module_dependencies
*
* @param array $module_dependencies The 'module_dependencies' of the classic script.
*/
public function test_print_script_module_translations_includes_classic_script_module_dependencies( array $module_dependencies ) {
$this->script_modules->register( 'static-dependency', '/wp-includes/js/static-dependency.js' );
$this->script_modules->register( 'dynamic-dependency', '/wp-includes/js/dynamic-dependency.js' );
$this->script_modules->register(
'imported-module',
'/wp-includes/js/imported-module.js',
array(
'static-dependency',
array(
'id' => 'dynamic-dependency',
'import' => 'dynamic',
),
)
);
wp_enqueue_script(
'classic',
'/classic.js',
array(),
false,
array(
'in_footer' => true,
'module_dependencies' => $module_dependencies,
)
);
$this->add_script_module_translations( array( 'imported-module', 'static-dependency', 'dynamic-dependency' ) );

$output = get_echo( array( $this->script_modules, 'print_script_module_translations' ) );

$this->assertEqualSets(
array( 'imported-module', 'static-dependency', 'dynamic-dependency' ),
$this->get_script_module_translation_ids( $output ),
'Translations should be printed once for the imported script module and for each of its dependencies.'
);
$this->assertStringContainsString( 'Hola desde imported-module', $output, 'Output should contain the translations of the imported script module.' );
}

/**
* Data provider.
*
* @return array<string, array{ 0: array<string|array<string, string>> }>
*/
public static function data_classic_script_module_dependencies(): array {
return array(
'module ID' => array( array( 'imported-module' ) ),
'static import' => array(
array(
array(
'id' => 'imported-module',
'import' => 'static',
),
),
),
'dynamic import' => array(
array(
array(
'id' => 'imported-module',
'import' => 'dynamic',
),
),
),
);
}

/**
* Tests that print_script_module_translations() outputs translations for the script module dependencies of a
* classic script that is only a dependency of the enqueued classic script.
*
* @covers WP_Script_Modules::print_script_module_translations
* @covers WP_Script_Modules::get_classic_script_module_dependencies
*/
public function test_print_script_module_translations_includes_module_dependencies_of_classic_script_dependencies() {
$this->script_modules->register( 'imported-module', '/wp-includes/js/imported-module.js' );
wp_register_script(
'classic-dependency',
'/classic-dependency.js',
array(),
false,
array(
'in_footer' => true,
'module_dependencies' => array( 'imported-module' ),
)
);
wp_enqueue_script( 'classic', '/classic.js', array( 'classic-dependency' ), false, array( 'in_footer' => true ) );
$this->add_script_module_translations( array( 'imported-module' ) );

$output = get_echo( array( $this->script_modules, 'print_script_module_translations' ) );

$this->assertSame( array( 'imported-module' ), $this->get_script_module_translation_ids( $output ) );
}

/**
* Tests that print_script_module_translations() outputs nothing for the script module dependencies of a classic
* script that is not enqueued.
*
* @covers WP_Script_Modules::print_script_module_translations
* @covers WP_Script_Modules::get_classic_script_module_dependencies
*/
public function test_print_script_module_translations_outputs_nothing_for_module_dependencies_of_classic_script_not_enqueued() {
$this->script_modules->register( 'imported-module', '/wp-includes/js/imported-module.js' );
wp_register_script(
'classic',
'/classic.js',
array(),
false,
array(
'in_footer' => true,
'module_dependencies' => array( 'imported-module' ),
)
);
$this->add_script_module_translations( array( 'imported-module' ) );

$output = get_echo( array( $this->script_modules, 'print_script_module_translations' ) );

$this->assertEmpty( $output );
}

/**
* Tests that print_script_module_translations() outputs the translations of a script module once when it is both
* enqueued and a script module dependency of a classic script.
*
* @covers WP_Script_Modules::print_script_module_translations
*/
public function test_print_script_module_translations_outputs_once_for_enqueued_classic_script_module_dependency() {
$this->script_modules->register( 'imported-module', '/wp-includes/js/imported-module.js' );
$this->script_modules->enqueue( 'imported-module' );
wp_enqueue_script(
'classic',
'/classic.js',
array(),
false,
array(
'in_footer' => true,
'module_dependencies' => array( 'imported-module' ),
)
);
$this->add_script_module_translations( array( 'imported-module' ) );

$output = get_echo( array( $this->script_modules, 'print_script_module_translations' ) );

$this->assertSame( array( 'imported-module' ), $this->get_script_module_translation_ids( $output ) );
}

/**
* Tests that a missing script module dependency of a classic script triggers a single _doing_it_wrong() notice,
* even though both the import map and the translations look the script module dependencies up.
*
* @covers WP_Script_Modules::get_classic_script_module_dependencies
* @covers WP_Script_Modules::print_import_map
* @covers WP_Script_Modules::print_script_module_translations
*/
public function test_missing_classic_script_module_dependency_triggers_doing_it_wrong_once() {
$this->setExpectedIncorrectUsage( 'WP_Scripts::add_data' );

$notices = 0;
add_action(
'doing_it_wrong_run',
static function ( $function_name ) use ( &$notices ) {
if ( 'WP_Scripts::add_data' === $function_name ) {
++$notices;
}
}
);
wp_enqueue_script(
'classic',
'/classic.js',
array(),
false,
array(
'in_footer' => true,
'module_dependencies' => array( 'does-not-exist' ),
)
);

get_echo( array( $this->script_modules, 'print_import_map' ) );
get_echo( array( $this->script_modules, 'print_script_module_translations' ) );

$this->assertSame( 1, $notices, 'The missing script module dependency should trigger a single notice.' );
}
}
Loading