Script Loader: Warn when classic scripts depend on modules without footer/defer #11788
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -885,7 +885,7 @@ public function add_data( $handle, $key, $value ) { | |
| sprintf( | ||
| /* translators: 1: $strategy, 2: $handle */ | ||
| __( 'Invalid strategy `%1$s` defined for `%2$s` during script registration.' ), | ||
| is_string( $value ) ? $value : gettype( $value ), | ||
|
Member
There was a problem hiding this comment. When looking at the PHPStan analysis, this inconsistency with the |
||
| $handle | ||
| ), | ||
| '6.3.0' | ||
|
|
@@ -897,7 +897,7 @@ public function add_data( $handle, $key, $value ) { | |
| sprintf( | ||
| /* translators: 1: $strategy, 2: $handle */ | ||
| __( 'Cannot supply a strategy `%1$s` for script `%2$s` because it is an alias (it lacks a `src` value).' ), | ||
| is_string( $value ) ? $value : gettype( $value ), | ||
|
Member
There was a problem hiding this comment.
westonruter marked this conversation as resolved.
|
||
| $handle | ||
| ), | ||
| '6.3.0' | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,19 +71,30 @@ function _wp_scripts_maybe_doing_it_wrong( $function_name, $handle = '' ) { | |
| /** | ||
| * Adds the data for the recognized args and warns for unrecognized args. | ||
| * | ||
| * @see wp_enqueue_script() | ||
| * @see wp_register_script() | ||
|
Comment on lines
+74
to
+75
Member
There was a problem hiding this comment. These are the only two functions which use this internal function. Again, something which would have been helpful to add when it was introduced in 6357346. |
||
| * | ||
| * @ignore | ||
| * @since 7.0.0 | ||
| * | ||
| * @param WP_Scripts $wp_scripts WP_Scripts instance. | ||
| * @param string $handle Script handle. | ||
| * @param array $args Array of extra args for the script. | ||
| * | ||
| * @phpstan-param non-empty-string $handle | ||
| * @phpstan-param array{ | ||
| * in_footer?: bool, | ||
| * strategy?: 'async'|'defer', | ||
| * fetchpriority?: 'low'|'auto'|'high', | ||
| * module_dependencies?: array<non-empty-string|array{ id: non-empty-string, ... }>, | ||
| * } $args | ||
|
Comment on lines
+84
to
+90
Member
There was a problem hiding this comment. This function is introduced in 7.0 but it lacks full PHPStan typing. This is a follow-up to 6357346. |
||
| */ | ||
| function _wp_scripts_add_args_data( WP_Scripts $wp_scripts, string $handle, array $args ): void { | ||
| $allowed_keys = array( 'strategy', 'in_footer', 'fetchpriority', 'module_dependencies' ); | ||
| $unknown_keys = array_diff( array_keys( $args ), $allowed_keys ); | ||
| if ( ! empty( $unknown_keys ) ) { | ||
| $trace = debug_backtrace( DEBUG_BACKTRACE_IGNORE_ARGS, 2 ); | ||
| $function_name = ( $trace[1]['class'] ?? '' ) . ( $trace[1]['type'] ?? '' ) . ( $trace[1]['function'] ?? __FUNCTION__ ); | ||
| _doing_it_wrong( | ||
| $function_name, | ||
| sprintf( | ||
|
|
@@ -97,7 +108,8 @@ function _wp_scripts_add_args_data( WP_Scripts $wp_scripts, string $handle, arra | |
| ); | ||
| } | ||
|
|
||
| $in_footer = ! empty( $args['in_footer'] ); | ||
| if ( $in_footer ) { | ||
| $wp_scripts->add_data( $handle, 'group', 1 ); | ||
| } | ||
| if ( ! empty( $args['strategy'] ) ) { | ||
|
|
@@ -108,6 +120,31 @@ function _wp_scripts_add_args_data( WP_Scripts $wp_scripts, string $handle, arra | |
| } | ||
| if ( ! empty( $args['module_dependencies'] ) ) { | ||
| $wp_scripts->add_data( $handle, 'module_dependencies', $args['module_dependencies'] ); | ||
|
|
||
| /* | ||
| * A classic script with module dependencies must either be printed in the | ||
| * footer or use the 'defer' loading strategy. Otherwise, the script may be | ||
| * evaluated before the script modules import map is printed, causing | ||
| * dynamic imports to fail with a "Failed to resolve module specifier" error. | ||
| */ | ||
| $is_deferred = 'defer' === ( $args['strategy'] ?? null ); | ||
| if ( ! $in_footer && ! $is_deferred ) { | ||
| $trace = debug_backtrace( DEBUG_BACKTRACE_IGNORE_ARGS, 2 ); | ||
| $function_name = ( $trace[1]['class'] ?? '' ) . ( $trace[1]['type'] ?? '' ) . ( $trace[1]['function'] ?? __FUNCTION__ ); | ||
| _doing_it_wrong( | ||
| $function_name, | ||
| sprintf( | ||
| /* translators: 1: 'module_dependencies', 2: Script handle, 3: 'in_footer', 4: 'strategy', 5: 'defer'. */ | ||
| __( 'When the %1$s arg is provided, the "%2$s" script must either be printed in the footer (%3$s set to true) or use a deferred loading %4$s (%5$s) so that the import map is printed before the script is evaluated.' ), | ||
| '<code>module_dependencies</code>', | ||
| $handle, | ||
| '<code>in_footer</code>', | ||
| '<code>strategy</code>', | ||
| '<code>defer</code>' | ||
| ), | ||
| '7.0.0' | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -204,25 +241,39 @@ function wp_add_inline_script( $handle, $data, $position = 'after' ) { | |
| * @since 6.9.0 The $fetchpriority parameter of type string was added to the $args parameter of type array. | ||
| * @since 7.0.0 The $module_dependencies parameter of type string[] was added to the $args parameter of type array. | ||
| * | ||
| * @param string $handle Name of the script. Should be unique. | ||
| * @param string|false $src Full URL of the script, or path of the script relative to the WordPress root directory. | ||
| * If source is set to false, script is an alias of other scripts it depends on. | ||
| * @param string[] $deps Optional. An array of registered script handles this script depends on. Default empty array. | ||
| * @param string|bool|null $ver Optional. String specifying script version number, if it has one, which is added to the URL | ||
| * as a query string for cache busting purposes. If version is set to false, a version | ||
| * number is automatically added equal to current installed WordPress version. | ||
| * If set to null, no version is added. | ||
| * @param array|bool $args { | ||
| * Optional. An array of extra args for the script. Default empty array. | ||
| * Otherwise, it may be a boolean in which case it determines whether the script is printed in the footer. Default false. | ||
| * | ||
| * @type string $strategy Optional. If provided, may be either 'defer' or 'async'. | ||
| * @type bool $in_footer Optional. Whether to print the script in the footer. Default 'false'. | ||
| * @type string $fetchpriority Optional. The fetch priority for the script. Default 'auto'. | ||
| * @type array $module_dependencies Optional. IDs for module dependencies loaded via dynamic import. Default empty array. | ||
|
Comment on lines
+244
to
+259
Member
There was a problem hiding this comment. I've simplified the types here to avoid the excessive indentation. This is a follow-up to 6357346 which is for 7.0. I've provided the full PHPStan types below, whereas they were previously incomplete by not specifying the array keys above (and so not very helpful). |
||
| * For the full data format, see the `$deps` param of {@see wp_register_script_module()}. | ||
| * When provided, the script must either be printed in the footer (with | ||
| * `in_footer` set to true) or use a deferred loading `strategy` (`defer`), | ||
| * so that the script modules import map is printed before the script | ||
| * is evaluated. Otherwise dynamic imports may fail to resolve. | ||
| * } | ||
| * @return bool Whether the script has been registered. True on success, false on failure. | ||
| * | ||
| * @phpstan-param non-empty-string $handle | ||
| * @phpstan-param non-empty-string|false $src | ||
| * @phpstan-param non-empty-string[] $deps | ||
| * @phpstan-param array{ | ||
| * in_footer?: bool, | ||
| * strategy?: 'async'|'defer', | ||
| * fetchpriority?: 'low'|'auto'|'high', | ||
| * module_dependencies?: array<non-empty-string|array{ id: non-empty-string, ... }>, | ||
| * }|bool $args | ||
| */ | ||
| function wp_register_script( $handle, $src, $deps = array(), $ver = false, $args = array() ) { | ||
| if ( ! is_array( $args ) ) { | ||
|
|
@@ -386,31 +437,46 @@ function wp_deregister_script( $handle ) { | |
| * @since 6.9.0 The $fetchpriority parameter of type string was added to the $args parameter of type array. | ||
| * @since 7.0.0 The $module_dependencies parameter of type string[] was added to the $args parameter of type array. | ||
| * | ||
| * @param string $handle Name of the script. Should be unique. | ||
| * @param string $src Full URL of the script, or path of the script relative to the WordPress root directory. | ||
| * Default empty. | ||
| * @param string[] $deps Optional. An array of registered script handles this script depends on. Default empty array. | ||
| * @param string|bool|null $ver Optional. String specifying script version number, if it has one, which is added to the URL | ||
| * as a query string for cache busting purposes. If version is set to false, a version | ||
| * number is automatically added equal to current installed WordPress version. | ||
| * If set to null, no version is added. | ||
| * @param array|bool $args { | ||
| * Optional. An array of extra args for the script. Default empty array. | ||
| * Otherwise, it may be a boolean in which case it determines whether the script is printed in the footer. Default false. | ||
| * | ||
| * @type string $strategy Optional. If provided, may be either 'defer' or 'async'. | ||
| * @type bool $in_footer Optional. Whether to print the script in the footer. Default 'false'. | ||
| * @type string $fetchpriority Optional. The fetch priority for the script. Default 'auto'. | ||
| * @type array $module_dependencies Optional. IDs for module dependencies loaded via dynamic import. Default empty array. | ||
| * For the full data format, see the `$deps` param of {@see wp_register_script_module()}. | ||
|
Comment on lines
+440
to
+456
Member
There was a problem hiding this comment. |
||
| * When provided, the script must either be printed in the footer (with | ||
| * `in_footer` set to true) or use a deferred loading `strategy` (`defer`), | ||
| * so that the script modules import map is printed before the script | ||
| * is evaluated. Otherwise dynamic imports may fail to resolve. | ||
| * } | ||
| * | ||
| * @phpstan-param non-empty-string $handle | ||
| * @phpstan-param string $src | ||
| * @phpstan-param non-empty-string[] $deps | ||
| * @phpstan-param array{ | ||
| * in_footer?: bool, | ||
| * strategy?: 'async'|'defer', | ||
| * fetchpriority?: 'low'|'auto'|'high', | ||
| * module_dependencies?: array<non-empty-string|array{ id: non-empty-string, ... }>, | ||
| * }|bool $args | ||
| */ | ||
| function wp_enqueue_script( $handle, $src = '', $deps = array(), $ver = false, $args = array() ) { | ||
| _wp_scripts_maybe_doing_it_wrong( __FUNCTION__, $handle ); | ||
|
|
||
| $wp_scripts = wp_scripts(); | ||
|
|
||
| if ( $src || ! empty( $args ) ) { | ||
| /** @var array{ 0: non-empty-string, 1?: string } $_handle */ | ||
|
Member
There was a problem hiding this comment. Technically this could be For that matter, the $_handle = strtok( $handle, '?' ); |
||
| $_handle = explode( '?', $handle ); | ||
| if ( ! is_array( $args ) ) { | ||
| $args = array( | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sirreal Thoughts on this? Because a full URL to a module is supplied, the
codemirrorscript indeed no longer has any dependency on the import map and it can freely be printed in theheadbefore theimportmapscript.In the future, when we support multiple
importmapscripts, we could return to usingmodule_dependenciesand importingespree.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One note here:
Before multiple importmaps (unsupported as of current Firefox 150), any module loading or preloading would invalidate subsequent importmaps. Firefox will print a warning and refuse to load an importmap. It's not sufficient to not use an imports, the act of loading (or preloading) a module triggers this behavior.
I don't think this is likely to be a problem in practice on the plugin pages, but we could defer and move them to the footer. This would also resolve the "practice what we preach" concern above. If the editor won't be initialized until DOMContentLoaded anyways, it doesn't seem like a big change.
Here's a demo:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alas, you're right. I was testing with Chrome which allows this:
If I swap the imports to be like this, however:
Then Chrome errors:
In Firefox, there is always this failure:
The error appearing in Chrome also appears in Firefox if I try importing
espreebefore theimportmapis seen.