Make WordPress Core

Opened 20 months ago

Closed 13 days ago

Last modified 6 days ago

#61500 closed enhancement (fixed)

Script Modules: Allow scripts to depend on modules

Reported by: jonsurrell's profile jonsurrell Owned by: westonruter's profile westonruter
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 jonsurrell)

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

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 by moduleId => true or moduleId => 'preload'. Only null is used at this time. This would also appear as an argument to the include_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

@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.

#9 @jonsurrell
5 months ago

  • Description modified (diff)

#10 @westonruter
2 months ago

  • Milestone changed from Awaiting Review to 7.0

#11 @westonruter
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-codemirror script module is registered with an empty string src. This module has espree added as a dynamic dependency. The empty string has the effect of preventing the wp-codemirror script module from being printed, while at the same time any of its dependencies still get added to the importmap.

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 modules key (or module_deps) that goes alongside strategy, in_footer, and fetchpriority?

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.

#15 @westonruter
3 weeks ago

  • Owner set to westonruter
  • Status changed from new to accepted

@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 up wp_register_script() and wp_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 $processed ensures 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

  1. 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_dependencies is well-formed.
    • The placeholder usage for $args in translation strings is correct.
  1. 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_wrong call correctly identifies missing module dependencies.
    • Usage of WP_Scripts::class . '::add' (or specifically WP_Scripts::add_data in the diff) as the function name in _doing_it_wrong is acceptable, though slightly unusual to see class constants used this way in legacy-style warnings.
  1. 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_dependencies is correct.
    • Assertions are specific and verify the expected behavior (import map contents, error notices).

### Nitpicks

  1. _doing_it_wrong context in WP_Script_Modules::get_import_map:

In the diff:

_doing_it_wrong(
        'WP_Scripts::add_data', // <--- This string
        // ...
    );

While WP_Scripts::add_data is technically where the data *was added*, the error is being detected during import map generation (enqueue time). A user might find it confusing to see WP_Scripts::add_data if they didn't explicitly call it (e.g. they called wp_enqueue_script). However, since wp_enqueue_script calls add_data internally via the new helper, this is technically accurate enough.

  1. _wp_scripts_add_args_data Documentation:

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.

#19 @westonruter
2 weeks ago

  • Keywords commit added

#21 @jonsurrell
2 weeks ago

  • Keywords needs-dev-note added

#22 @westonruter
13 days ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 61587:

Script Loader: Allow classic scripts to depend on script modules.

This allows classic scripts to declare dependencies on script modules by passing module_dependencies in the $args param for wp_register_script() or wp_enqueue_script(). The WP_Script_Modules::get_import_map() method is updated to traverse the dependency tree of all enqueued classic scripts to find any associated script module dependencies and include them in the importmap, enabling dynamic imports of modules within classic scripts.

A _wp_scripts_add_args_data() helper function is introduced to consolidate argument validation and processing for wp_register_script() and wp_enqueue_script(), reducing code duplication. This function validates that the $args array only contains recognized keys (strategy, in_footer, fetchpriority, module_dependencies) and triggers a _doing_it_wrong() notice for any unrecognized keys. Similarly, WP_Scripts::add_data() is updated to do early type checking for the data passed to $args. The script modules in module_dependencies may be referenced by a module ID string or by an array that has an id key, following the same pattern as dependencies in WP_Script_Modules.

When a script module is added to the module_dependencies for a classic script, but it does not exist at the time the importmap is printed, a _doing_it_wrong() notice is emitted.

Developed in https://github.com/WordPress/wordpress-develop/pull/8024

Follow-up to [61323].

Props sirreal, westonruter.
See #64229.
Fixes #61500.

#23 @jonsurrell
13 days ago

I've created issue 75196 to update build tooling accordingly to detect module dependencies of scripts.

#24 @westonruter
6 days ago

In 61611:

Code Editor: Switch from Esprima to Espree for JavaScript linting in CodeMirror.

Esprima is no longer maintained, and it does not support the latest JavaScript features in ES11, as Espree does.

  • New Linter Integration: Introduces src/js/_enqueues/vendor/codemirror/javascript-lint.js using espree for parsing and error reporting, replacing the dependency on jshint and esprima scripts.
  • Script Modules: Registers espree as a script module and leverages the module_dependencies argument in wp_register_script() to ensure espree is available as a dynamic import.
  • Editor Settings: Updates wp_get_code_editor_settings() to use ES11 (ECMAScript 2020) defaults and synchronizes JSHint settings from .jshintrc for compatibility.
  • Editable Extensions: Adds .mjs to the list of editable file extensions for plugins and themes.
  • Deprecations: Marks esprima and jshint script handles as deprecated.
  • Build Tools: Updates Webpack configuration to bundle espree as a module and use the new local javascript-lint.js.

Developed in https://github.com/WordPress/wordpress-develop/pull/10806

Follow-up to [61587], [61544], [61539], [42547].

Props westonruter, jonsurrell.
See #64562, #61500, #48456, #42850.
Fixes #64558.

Note: See TracTickets for help on using tickets.