#61500 closed enhancement (fixed)
Script Modules: Allow scripts to depend on modules
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 7.0 | Priority: | normal |
| Severity: | normal | Version: | 6.5 |
| Component: | Script Loader | Keywords: | has-patch has-unit-tests commit needs-dev-note |
| Focuses: | javascript, performance | Cc: |
Description (last modified by )
WordPress supports JavaScript modules via the Script Modules API.
Classic Scripts would benefit from the use of Script Modules to take advantage of some benefits like conditional and deferred loading, but classic Scripts have no way of declaring a dependency on Script Modules.
Classic scripts can use the "dynamic" import( 'module-id' ) to import JavaScript modules, but there's currently no way to declare the dependency so that dependent modules are included in the import map, making this usage problematic in WordPress.
Ideally, Scripts should be able declare Script Module dependencies so that scripts can safely use modules.
This complements #60647 which seeks to add more functionality to Script Modules.
Change History (24)
This ticket was mentioned in PR #8009 on WordPress/wordpress-develop by @jonsurrell.
14 months ago
#1
- Keywords has-patch has-unit-tests added
@jonsurrell commented on PR #8009:
14 months ago
#2
@luisherranz I'd love your thoughts on this.
@youknowriad @swissspidy This is one of the first pieces towards allowing scripts to depend on modules, I know that you're working on functionality that would like to leverage this. I'd appreciate your thoughts.
@jonsurrell commented on PR #8009:
14 months ago
#3
I added a test and fixed an issues where regular dependencies and "requested for inclusion" modules were being incorrectly overwritten.
@jonsurrell commented on PR #8009:
14 months ago
#4
https://github.com/WordPress/wordpress-develop/pull/8024 implements a complete system utilizing this.
I'm not too happy with the result. It requires scripts to print before the importmap which is not reliable.
It may be better to have scripts continue to declare module dependencies, but when printing the import map have the module system query enqueued scripts and print their module dependencies in the importmap.
I'll make this PR a draft while I continue to explore a complete implementation.
@luisherranz commented on PR #8009:
14 months ago
#5
@luisherranz I'd love your thoughts on this.
Code-wise, it looks good to me. The only question I have is why you decided to use an external array instead of a flag in the properties of the registered script module (like enqueue).
This ticket was mentioned in PR #8024 on WordPress/wordpress-develop by @jonsurrell.
14 months ago
#6
Allow classic scripts to depend on script modules.
This change proposes allowing a module dependencies to classic scripts.
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 https://github.com/WordPress/wordpress-develop/pull/8009.~ The implementation here is now independent.
Trac ticket: https://core.trac.wordpress.org/ticket/61500
@jonsurrell commented on PR #8009:
14 months ago
#7
I've found an approach in https://github.com/WordPress/wordpress-develop/pull/8024 that seems like a better solution and does not require this work. I'll close this PR. If folks think there's still value in adding this method we can reopen and consider it.
@jonsurrell commented on PR #8024:
14 months ago
#8
@luisherranz @youknowriad @swissspidy @gziolo I'd love to have your review on this approach to allowing classic scripts to depend on script modules.
#11
@
3 weeks ago
I just left a comment in #48456 with a workaround:
Note that since classic scripts cannot explicitly depend on script modules (#61500), this is currently implemented as a bit of a hack. A new
wp-codemirrorscript module is registered with an empty stringsrc. This module hasespreeadded as a dynamic dependency. The empty string has the effect of preventing thewp-codemirrorscript module from being printed, while at the same time any of its dependencies still get added to theimportmap.
This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.
3 weeks ago
@westonruter commented on PR #8024:
3 weeks ago
#13
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', ), ), );
@sirreal I've been giving this some thought. I have some concerns about overloading the $deps arg to allow intermingling classic script handles and new module identifiers. This could cause problems with ecosystem plugins that are doing things with $deps and expecting them to only be strings. For example, the imagify plugin has code like this:
$dependencies = array_flip( $dependencies->deps );
if ( ! isset( $dependencies['heartbeat'] ) ) {
return;
}
This causes a PHP warning if any of the $deps is not a string:
Warning: array_flip(): Can only flip string and integer values, entry skipped
This being the case, how about an alternative proposal of introducing a new modules key (or module_deps) that goes alongside strategy, in_footer, and fetchpriority? Since these will only ever be dynamic imports, there's no need to differentiate from static. So the above example can be redone as:
wp_register_script_module( '@example/module', /*…*/ );
wp_enqueue_script(
'example-classic',
'/classic.js',
array(
'classic-script-dependency',
),
null, // Version.
array(
'module_deps' => array( '@example/module' ),
// Plus 'in_footer', 'strategy', etc.
)
);
This to me seems much simpler and backwards-compatible.
Note: When modules are marked as dependencies of scripts, we should also warn when they are missing per Core-64229.
@jonsurrell commented on PR #8024:
3 weeks ago
#14
how about an alternative proposal of introducing a new
moduleskey (ormodule_deps) that goes alongsidestrategy,in_footer, andfetchpriority?
These changes are not fresh in my mind, but that seems like a good proposal in general 👍
---
Since I started this work, multiple import maps were finalized and landed in most major browsers with the exception of Firefox. Eventually, that may remove the timing constraints around needing to know what scripts will be present in order to include their module dependencies when the importmap is printed.
@westonruter commented on PR #8024:
3 weeks ago
#16
OK, I think the merge conflicts have been resolved to bring it back up to date with trunk. With the tests now passing, I'll proceed with refactoring to introduce the module_deps arg (er, maybe module_dependnecies).
@westonruter commented on PR #8024:
2 weeks ago
#17
_Gemini review:_
Review of changes in scripts/allow-script-module-dependency
The changes introduce logic to include script module dependencies defined in classic scripts (via the module_dependencies key) into the import map. It also adds validation for the $args parameter in script registration functions and refactors them to reduce duplication.
### General Observations
- Standards & Compatibility: The code generally adheres to WordPress Coding Standards. The usage of typed properties in tests (PHP 7.4+) is acceptable for the current supported versions in the test suite.
- Refactoring: The extraction of
_wp_scripts_add_args_data()significantly cleans upwp_register_script()andwp_enqueue_script(), reducing duplication and centralizing validation. - Logic: The dependency traversal in
WP_Script_Modules::get_import_map()uses an iterative approach with a stack (array_pop), which is efficient and avoids recursion depth issues. The use of$processedensures circular dependencies don't cause infinite loops.
### Conclusion
The changes are high quality, verified by tests, and improve the codebase's maintainability while adding the requested feature. Addressing the typo in the comment is the only immediate action recommended.
@westonruter commented on PR #8024:
2 weeks ago
#18
_Updated Gemini review (with nitpicks not needing actioning):_
Review of changes in scripts/allow-script-module-dependency
The changes introduce functionality to allow classic scripts to declare dependencies on script modules, including validation, processing, and handling within the import map generation. The implementation also refactors script registration logic to reduce duplication.
### Observations
- Refactoring (
functions.wp-scripts.php):- The introduction of
_wp_scripts_add_args_data()is a solid improvement, centralizing validation and data addition. - Validation logic correctly checks for unknown keys and ensures
module_dependenciesis well-formed. - The placeholder usage for
$argsin translation strings is correct.
- The introduction of
- Script Module Integration (
WP_Script_Modules::get_import_map):- The logic to traverse classic script dependencies (
$wp_scripts->registered[$handle]->deps) to find associated module dependencies is implemented iteratively with a stack (array_pop). This avoids recursion limits and is efficient. - The
_doing_it_wrongcall correctly identifies missing module dependencies. - Usage of
WP_Scripts::class . '::add'(or specificallyWP_Scripts::add_datain the diff) as the function name in_doing_it_wrongis acceptable, though slightly unusual to see class constants used this way in legacy-style warnings.
- The logic to traverse classic script dependencies (
- Tests (
Tests_Script_Modules_WpScriptModules):- The tests are comprehensive, covering direct module dependencies, transitive dependencies, and error conditions.
- The typo fix "dependnecy" -> "dependency" in
test_wp_scripts_doing_it_wrong_for_missing_script_module_dependenciesis correct. - Assertions are specific and verify the expected behavior (import map contents, error notices).
### Nitpicks
-
_doing_it_wrongcontext inWP_Script_Modules::get_import_map:
In the diff:
_doing_it_wrong( 'WP_Scripts::add_data', // <--- This string // ... );
While
WP_Scripts::add_datais technically where the data *was added*, the error is being detected during import map generation (enqueue time). A user might find it confusing to seeWP_Scripts::add_dataif they didn't explicitly call it (e.g. they calledwp_enqueue_script). However, sincewp_enqueue_scriptcallsadd_datainternally via the new helper, this is technically accurate enough.
-
_wp_scripts_add_args_dataDocumentation:
The docblock says:
* Adds the data for the recognized args and warns for unrecognized args.
This follows the third-person singular verb standard requested.
### Conclusion
The code is robust, follows WordPress coding standards, and the test coverage is thorough. The refactoring improves maintainability.
Plan:
No changes are required. The current state is ready.
@westonruter commented on PR #8024:
2 weeks ago
#20
I squash-merged this PR into https://github.com/WordPress/wordpress-develop/pull/10806 to kick the tires. I added https://github.com/WordPress/wordpress-develop/pull/10806/changes/34f5e4c6d7204f03e7447dd46ed4973e004ba35a to make use of it to add espree as a dependency of wp-codemirror. It works!
#23
@
13 days ago
I've created issue 75196 to update build tooling accordingly to detect module dependencies of scripts.
This is a first step towards 61500, it adds a method to the script modules class to allow modules to be flagged for inclusion in the import map.
This means that a module can be flagged for inclusion and it will appear in the import map regardless of whether it's absent from the script module dependency graph.
This can be useful for developers to force modules to appear in the import map where they can be imported on the browser console. I expect this method to be used in the future so that classic scripts can create a dependency on script modules, where this new method will act the script dependency system to request that modules appear in the importmap.
Internally it uses an array:
moduleId => null. The value is always null, but other values could be used, e.g. to request that it appear in the import map _and_ in module preloads the value might bymoduleId => trueormoduleId => 'preload'. Onlynullis used at this time. This would also appear as an argument to theinclude_in_import_map` method, but again that is not implemented at this time and only presented for consideration.Trac ticket: https://core.trac.wordpress.org/ticket/61500