Repository navigation
I18N: Print translations for the script modules that scripts depend on - #14260
jorgefilipecosta wants to merge 5 commits into
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. |
| /* | ||
| * 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 ) ) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
print_script_module_data() already builds "every module that can load" from the queue plus the import map keys:
wordpress-develop/src/wp-includes/class-wp-script-modules.php
Lines 1063 to 1076 in 4fa8850
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).
| /* | |
| * 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'] ) | |
| ) | |
| ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hi @manzoorwanijk, I applied your suggestion, the translations now use the queue plus the import map keys, the extraction was removed and the once per script notice check was kept 👍
There was a problem hiding this comment.
Hi @jsnajdr, both loops now get the script modules from the same get_loadable_script_module_ids method, so they always match.
| * @since 7.2.0 | ||
| * @var string[] | ||
| */ | ||
| private $classic_scripts_with_missing_module_dependencies = array(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good point, it was updated, the code, the tests, the commits and the PR description now say scripts 👍
|
I tested by rebasing this PR onto latest <?php
add_filter( 'pre_load_script_translations', function ( $translations, $file, $handle ) {
if ( ! str_starts_with( $handle, '@wordpress/core-actions/' ) && ! str_starts_with( $handle, '@wordpress/core-fields/' ) ) {
return $translations;
}
$strings = array(
// Actions (post_type_supports module).
'Name updated' => 'Name updated (translated)',
'Are you sure you want to move "%s" to the trash?' => 'TRANSLATED: move "%s" to the trash?',
// Fields (post_type_supports module, Date field).
'<span>Modified: <time>%s</time></span>' => '<span>Modified (translated): <time>%s</time></span>',
'<span>Published: <time>%s</time></span>' => '<span>Published (translated): <time>%s</time></span>',
'<span>Scheduled: <time>%s</time></span>' => '<span>Scheduled (translated): <time>%s</time></span>',
);
$messages = array( '' => array() );
foreach ( $strings as $from => $to ) {
$messages[ $from ] = array( $to );
}
return wp_json_encode( array( 'locale_data' => array( 'messages' => $messages ) ) );
}, 10, 3 );Then, verifying those translations are displayed (they are not in
|
4fa8850 to
c95b458
Compare
`WP_Script_Modules::print_script_module_translations()` only covered the enqueued script modules and their dependencies. A script module that only a script imports, declared in the script's `module_dependencies`, is added to the import map and loads, but its translations were never printed, so its strings showed untranslated. The method now covers the enqueued script modules and the script modules in the import map, which includes the script module dependencies of scripts and their dependencies. `WP_Script_Modules::print_script_module_data()` already covered these script modules, and both methods now get them from the same private `get_loadable_script_module_ids()` method, which sorts them with `get_sorted_dependencies()`. This way both methods always cover the same script modules, in the same order, with dependencies before the script modules that depend on them. A script module that is both enqueued and imported by a script gets its translations printed once. Since the import map is now built three times per page, a missing script module dependency of a script triggers the `_doing_it_wrong()` notice once per script. Fixes #66591.
…Script_Modules::get_import_map()`. Renames the variable that holds the script module dependencies of scripts to `$script_module_dependencies` and updates the comments to say "scripts", the name used in the rest of the codebase. There is no change in behavior. See #66591.
c95b458 to
c2f21ff
Compare
…data are printed for. `WP_Script_Modules::get_loadable_script_module_ids()` sorted the script modules with `get_sorted_dependencies()`. That method leaves out a script module that has an unregistered dependency, although the script module is in the import map and still loads, and it does not finish when the dependencies are circular. The method now returns the enqueued script modules followed by the script modules in the import map, without sorting them. `print_script_module_data()` covers the same script modules as before this ticket, and `print_script_module_translations()` covers the same ones. Removes the check that limited the `_doing_it_wrong()` notice for a missing script module dependency of a script to once per script. The notice is triggered each time the import map is built, as before, which now happens three times per page. Folds the tests for the script module dependencies of scripts into a data provider, and adds tests for unregistered and circular dependencies. See #66591.
| return array_unique( | ||
| array_merge( | ||
| $this->queue, | ||
| array_keys( $this->get_import_map()['imports'] ) |
There was a problem hiding this comment.
This is a previous issue, it can be a follow-up optimization.
I've realized get_import_map calls doing_it_wrong. On trunk, it already runs twice per page (print_script_module_data and print_import_map), with this change it'll run three times.
There was a problem hiding this comment.
Yes, the notice is now triggered once per script, so it does not repeat.
| 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 ); | ||
| $module_ids = $this->get_loadable_script_module_ids(); |
There was a problem hiding this comment.
Besides changing which modules get translations (the fix we need: queue + import map), this also changes the order they are printed (before: sorted, dependencies first / after: queue + import map).
Since every module calls wp.i18n.setLocaleData() on the same domain, the last one printed wins when two modules translate the same string differently. For example:
// my-panel depends on my-dialog, buth use __( 'Close' ) ni the default domain.
wp_register_script_module( 'my-dialog', $url . 'dialog.js' );
wp_register_script_module( 'my-panel', $url . 'panel.js', array( 'my-dialog' ) );
wp_enqueue_script_module( 'my-panel' );
// Something overrides my-panel's translations only:
add_filter( 'pre_load_script_translations', function ( $json, $file, $handle ) {
if ( 'my-panel' === $handle ) {
return '{"locale_data":{"messages":{"":{"lang":"pt"},"Close":["Sair"]}}}';
}
return $json;
}, 10, 3 );Considering my-dialog's language-pack JSON has "Close": ["Fechar"], this is the before/after result:
| Order printed | Last setLocaleData |
__( 'Close' ) returns |
|
|---|---|---|---|
| before: trunk | my-dialog, my-panel |
my-panel |
Sair |
| after: pr | my-panel, my-dialog |
my-dialog |
Fechar |
I don't think this happens with language packs: a string with the same context has one translation per project, so every module's JSON agrees. It takes a per-module override like the one above, or a custom path set via set_translations(). So I'd be fine with documenting the new order in the docblock.
Alternatively, we could considering adding back sorting (but differently). I see in 8951e00 that removing get_sorted_dependencies() was intentional.
There was a problem hiding this comment.
I see in 8951e00 that removing
get_sorted_dependencies()was intentional.
I think that the main criterion here is that classic scripts and script modules should use the same algorithm for the order in which they are printed. And classic scripts are always sorted.
The commit message on 8951e00 says:
WP_Script_Modules::get_loadable_script_module_ids()sorted the script modules withget_sorted_dependencies(). That method leaves out a script module that has an unregistered dependency, although the script module is in the import map and still loads, and it does not finish when the dependencies are circular.
Yes, but that's the same behavior that classic scripts have. The sorting code eliminates unregistered dependencies. Also also apparently triggers an infinite loop for circular dependencies?!
If script modules behave differently, that's an accidental behavior that should not be preserved, but fixed. If there are really crashing circular dependencies, that's a topic for another patch.
There was a problem hiding this comment.
Nice catch, I documented the new order in the docblock of print_script_module_translations, including that the translation printed last is the one used.
There was a problem hiding this comment.
I think that the main criterion here is that classic scripts and script modules should use the same algorithm for the order in which they are printed.
Hi @jsnajdr, I agree that is where we should get to. I did not sort in this PR because print_script_module_data never sorted. Sorting the shared list would stop printing the data of script modules that have an unregistered dependency, which is printed today, and it would not finish for circular dependencies between script modules that scripts import, which work today. I think it is better to fix get_sorted_dependencies first and then sort all the printers in a follow-up.
|
Another thing the old wp_register_script_module( 'my-utils', $url . 'utils.js' );
wp_register_script_module( 'my-widget', $url . 'widget.js', array( 'my-utils', 'my-typo' ) ); // 'my-typo' is not registered
wp_enqueue_script_module( 'my-widget' );I've noticed each printer of this class does its own thing:
This could be streamlined, though the core issue (every printer does its own thing) is previous to this branch, so I don't think it's a blocker for this pull request. |
oandregal
left a comment
There was a problem hiding this comment.
This is working as expected.
I've left some comments for improvements but they are not blockers (they are pre-existing).
…ce per script. `WP_Script_Modules::get_import_map()` triggers a `_doing_it_wrong()` notice when a script has a script module dependency that is not registered. The import map is built three times per page, for the import map itself, for the translations and for the data, so the notice was triggered three times for the same script. The handles of the scripts that already triggered the notice are now kept, as is done for the script modules with missing dependencies, and the notice is triggered once per script. Documents in `WP_Script_Modules::print_script_module_translations()` the order in which the translations are printed: the enqueued script modules first, followed by the script modules in the import map. When two script modules translate the same string of a text domain differently, the translation printed last is the one that is used. See #66591.
…fter the footer scripts. `WP_Script_Modules::print_script_module_translations()` runs after the scripts in the footer are printed. A script that imports a script module while it executes may evaluate the script module before its translations are printed. This is now documented in the method, in `WP_Script_Modules::add_hooks()`, and in the `module_dependencies` arg of `wp_register_script()` and `wp_enqueue_script()`, together with the ways to avoid it: use the `defer` loading strategy, or import the script module once the document has been parsed. There is no change in behavior. See #66591.
|
Hi @oandregal, thank you for testing it with the actions and fields script modules and for sharing the snippet 😊 |
|
Thank you @oandregal and @t-hamano, the ticket is linked in the PR description and the commits reference it. |
Hi @oandregal, I agree. With this PR the translations follow the same list as the data, so in that example both are printed for my-widget and my-utils. I think making the module tags, the import map, the data and the translations follow the same rules is a good follow-up. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. 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. Props Bot commandsYou can trigger Props Bot actions by commenting on this PR:
|
`WP_Script_Modules::print_script_module_translations()` only printed the translations of the enqueued script modules and their dependencies. A script module that only a script imports, declared in the script's `module_dependencies`, is added to the import map and loads, but its translations were never printed, so its strings showed untranslated. This is the case of the `@wordpress/core-actions` and `@wordpress/core-fields` script modules that the `wp-editor` script imports. The method now prints the translations of the enqueued script modules, followed by the translations of the script modules in the import map, which include the script module dependencies of scripts and their dependencies. `WP_Script_Modules::print_script_module_data()` already covered these script modules, and both methods now get them from the new private `WP_Script_Modules::get_loadable_script_module_ids()` method. Developed in #14260. Follow-up to [61587], [62278]. Props jorgefilipecosta, manzoorwanijk, jsnajdr, oandregal, wildworks. Fixes #66591. git-svn-id: https://develop.svn.wordpress.org/trunk@65411 602fd350-edb4-49c9-b593-d223f7449a82
`WP_Script_Modules::print_script_module_translations()` only printed the translations of the enqueued script modules and their dependencies. A script module that only a script imports, declared in the script's `module_dependencies`, is added to the import map and loads, but its translations were never printed, so its strings showed untranslated. This is the case of the `@wordpress/core-actions` and `@wordpress/core-fields` script modules that the `wp-editor` script imports. The method now prints the translations of the enqueued script modules, followed by the translations of the script modules in the import map, which include the script module dependencies of scripts and their dependencies. `WP_Script_Modules::print_script_module_data()` already covered these script modules, and both methods now get them from the new private `WP_Script_Modules::get_loadable_script_module_ids()` method. Developed in WordPress/wordpress-develop#14260. Follow-up to [61587], [62278]. Props jorgefilipecosta, manzoorwanijk, jsnajdr, oandregal, wildworks. Fixes #66591. Built from https://develop.svn.wordpress.org/trunk@65411 git-svn-id: http://core.svn.wordpress.org/trunk@64550 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Right now
WP_Script_Modules::print_script_module_translations()only prints the translations of the enqueued script modules and their dependencies. Since WordPress 7.0, a script can also declare the script modules it imports in itsmodule_dependenciesdata, andget_import_map()adds those script modules and their dependencies to the import map. So a script module that only a script imports loads fine, but its translations are never printed, and its strings show in English even when the language pack translates them.This PR:
get_loadable_script_module_ids()method that returns the enqueued script modules followed by the script modules in the import map.print_script_module_translations()andprint_script_module_data()now loop over that same list, so the translations, the data and the import map always cover the same script modules.print_script_module_translations()also print the translations of the script modules that scripts depend on, and of their dependencies. A script module that is both enqueued and a script's dependency is printed once._doing_it_wrong()notice for missing script modules fire once per script. The import map is already built twice per page, byprint_import_map()andprint_script_module_data(), so the notice fired twice, and the new caller would make it three times.Some notes:
print_script_module_translations()documents the order.srcis not in the import map. So when it is only a dependency of an enqueued script module, its translations are no longer printed.@wordpress/core-actionsand@wordpress/core-fieldsscript modules that thewp-editorscript imports through itsmodule_dependencies. On trunk their translations are not printed, so strings like the confirmation message of the "Trash" action in the Pages list of the Site Editor show in English. The same applies to the script modules that plugins register for their actions and fields.Before
The test page of the e2e test in the Gutenberg backport, where one line comes from an enqueued script module and the other from a script module that only a script imports:
After
Testing Instructions
If Gutenberg is active, deactivate it, since Script Modules: Fix missing translations of script modules gutenberg#84651 fixes the same issue in the plugin.
Save this snippet as
wp-content/mu-plugins/module-translations-test.php(src/wp-content/mu-plugins/in a wordpress-develop checkout). It registers atest/helloscript module that only a script imports, gives it a translation, and makes it show__( 'Hello' )in an alert on the Dashboard:Open the Dashboard. On trunk, the alert says "Hello". With this PR, it says "Hello (translated)".
Delete the snippet and run
npm run test:php -- --group script-modules.Gutenberg backport: WordPress/gutenberg#84651.
Trac ticket: https://core.trac.wordpress.org/ticket/66591
Use of AI Tools
AI assistance: Yes
Used for: drafting the fix, the tests and this description; reviewed and edited by me.
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.