Skip to content

Improve handling of delayed strategies when dependency aliases (bundles) are involved #80

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
westonruter merged 12 commits into from
Jun 16, 2023
Merged

Improve handling of delayed strategies when dependency aliases (bundles) are involved #80

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e499918
Refer to aliases instead of bundles
westonruter Jun 16, 2023
f3d3c44
Improve handling of dependency aliases
westonruter Jun 16, 2023
b06affe
Add key exists check to mirror parent add_data method
westonruter Jun 16, 2023
bbec078
Add test case to show deferral of jquery
westonruter Jun 16, 2023
47b2cbc
Use isset() instead of array_key_exists() for perf
westonruter Jun 16, 2023
805370e
Use more isset() instead of array_key_exists() for perf
westonruter Jun 16, 2023
c28fa9a
Simplify conditionals by using ternary
westonruter Jun 16, 2023
e72fcc0
Factor out logic into has_inline_script helper method
westonruter Jun 16, 2023
57582d7
Account for aliases having nested aliases
westonruter Jun 16, 2023
4af0d23
Avoid needlessly recursing among dependencies when only looking at de…
westonruter Jun 16, 2023
9f66c06
Add test ensuring that an async alias gets converted to defer when th…
westonruter Jun 16, 2023
9cb97ab
Remove erroneous expected delayed-inline-script-loader
westonruter Jun 16, 2023
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
112 changes: 92 additions & 20 deletions src/wp-includes/class-wp-scripts.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,33 @@ public function get_inline_script_data( $handle, $position = 'after' ) {
return trim( implode( "\n", $data ), "\n" );
}

/**
* Gets unaliased dependencies.
*
* An alias is a dependency whose src is false. It is used as a way to bundle multiple dependencies in a single
* handle. This in effect flattens an alias dependency tree.
*
* @since 6.3.0
*
* @param string[] $deps Dependency handles.
* @return string[] Unaliased handles.
*/
private function get_unaliased_deps( array $deps ) {
$flattened = array();
foreach ( $deps as $dep ) {
if ( ! isset( $this->registered[ $dep ] ) ) {
continue;
}

if ( $this->registered[ $dep ]->src ) {
$flattened[] = $dep;
} elseif ( $this->registered[ $dep ]->deps ) {
array_push( $flattened, ...$this->get_unaliased_deps( $this->registered[ $dep ]->deps ) );
Copy link
Copy Markdown
Collaborator Author

@westonruter westonruter Jun 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Makes me so happy that the ... operator is supported in PHP 5.6+.

Copy link
Copy Markdown

@10upsimon 10upsimon Jun 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ya, this is great. Worth noting - for reference sake - that implementation of the spread operator differs from 5.6 - 7.x in the sense that 5.6 only supports argument unpacking, but not within array expressions. Thankfully, that is all that's needed here :)

}
}
return $flattened;
}

/**
* Gets tags for inline scripts registered for a specific handle.
*
Expand All @@ -568,14 +595,20 @@ public function get_inline_script_tag( $handle, $position = 'after' ) {
return '';
}

$id = "{$handle}-js-{$position}";
$deps = $this->registered[ $handle ]->deps;

$id = "{$handle}-js-{$position}";
if ( $this->should_delay_inline_script( $handle, $position ) ) {
$attributes = array(
'id' => $id,
'type' => 'text/plain',
);

/*
* Note that any dependency aliases need to be flattened because an alias is a bundle of dependencies
* and their handles won't appear on any specific scripts. Since no script will appear in the DOM for
* an alias, there won't be any way to keep track of when it has loaded. Therefore, we only keep track of
* the aliased dependencies (the leaf nodes of the alias dependency tree as it were).
*/
$deps = $this->get_unaliased_deps( $this->registered[ $handle ]->deps );
if ( $deps ) {
$attributes['data-wp-deps'] = implode( ',', $deps );
}
Expand Down Expand Up @@ -648,7 +681,18 @@ private function should_delay_inline_script( $handle, $position ) {
* dependency's after inline script.
*/
foreach ( $deps as $dep ) {
if ( $this->is_delayed_strategy( $this->get_eligible_loading_strategy( $dep ) ) ) {
if ( ! isset( $this->registered[ $dep ] ) ) {
continue;
}

// If the dependency is an alias, look at its members.
if ( ! $this->registered[ $dep ]->src ) {
foreach ( $this->get_unaliased_deps( $this->registered[ $dep ]->deps ) as $alias_dep ) {
if ( $this->is_delayed_strategy( $this->get_eligible_loading_strategy( $alias_dep ) ) ) {
return true;
}
}
} elseif ( $this->is_delayed_strategy( $this->get_eligible_loading_strategy( $dep ) ) ) {
return true;
}
}
Expand Down Expand Up @@ -911,6 +955,10 @@ public function in_default_dir( $src ) {
* @return bool True on success, false on failure.
*/
public function add_data( $handle, $key, $value ) {
if ( ! isset( $this->registered[ $handle ] ) ) {
return false;
}

if ( 'strategy' === $key ) {
if ( ! empty( $value ) && ! $this->is_delayed_strategy( $value ) ) {
_doing_it_wrong(
Expand All @@ -924,12 +972,12 @@ public function add_data( $handle, $key, $value ) {
'6.3.0'
);
return false;
} elseif ( empty( $this->registered[ $handle ]->src ) && $this->is_delayed_strategy( $value ) ) {
} elseif ( ! $this->registered[ $handle ]->src && $this->is_delayed_strategy( $value ) ) {
_doing_it_wrong(
__METHOD__,
sprintf(
/* translators: 1: $strategy, 2: $handle */
__( 'Cannot supply a strategy `%1$s` for script `%2$s` because it does not have a `src` value.' ),
__( 'Cannot supply a strategy `%1$s` for script `%2$s` because it is an alias (it lacks a `src` value).' ),
$value,
$handle
),
Expand Down Expand Up @@ -974,7 +1022,7 @@ public function has_delayed_inline_script( array $handles ) {
*/
private function get_dependents( $handle ) {
// Check if dependents map for the handle in question is present. If so, use it.
if ( array_key_exists( $handle, $this->dependents_map ) ) {
if ( isset( $this->dependents_map[ $handle ] ) ) {
return $this->dependents_map[ $handle ];
}

Expand Down Expand Up @@ -1021,38 +1069,44 @@ private function is_delayed_strategy( $strategy ) {
*/
private function has_only_delayed_dependents( $handle, $async_only = false, $checked = array() ) {
// If this node was already checked, this script can be delayed and the branch ends.
if ( array_key_exists( $handle, $checked ) ) {
if ( isset( $checked[ $handle ] ) ) {
return true;
}

$checked[ $handle ] = true;
$dependents = $this->get_dependents( $handle );

// If there are no dependents remaining to consider, the script can be deferred.
// If there are no dependents remaining to consider, the script can be delayed.
if ( empty( $dependents ) ) {
return true;
}

// Consider each dependent and check if it is delayed.
foreach ( $dependents as $dependent ) {
// If the dependent script has no src (as it represents a script bundle), ignore it for consideration.
if ( empty( $this->registered[ $dependent ]->src ) ) {
if ( ! isset( $this->registered[ $dependent ] ) ) {
continue;
}

// If the dependency is not enqueued, ignore it for consideration.
// If the dependency is not enqueued, exclude it from consideration.
if ( ! $this->query( $dependent, 'enqueued' ) ) {
continue;
}

// If the dependent script is not using the defer or async strategy, no script in the chain is delayed.
$strategy = $this->get_data( $dependent, 'strategy' );
if ( $async_only ) {
if ( 'async' !== $strategy ) {
// Handle script alias case (where it has no src). Here, the strategy doesn't matter, but only whether there are inline scripts.
if ( ! $this->registered[ $dependent ]->src ) {
// A script alias cannot be delayed if it has inline scripts since there is no load event.
if ( $this->has_inline_script( $dependent ) ) {
return false;
}
} else {
// If the dependent script is not using the defer or async strategy, no script in the chain is delayed.
$strategy = $this->get_data( $dependent, 'strategy' );
if ( $async_only ?
'async' !== $strategy :
! $this->is_delayed_strategy( $strategy )
) {
return false;
}
} elseif ( ! $this->is_delayed_strategy( $strategy ) ) {
return false;
}

// Recursively check all dependents.
Expand Down Expand Up @@ -1082,10 +1136,15 @@ private function get_eligible_loading_strategy( $handle ) {
/*
* Handle known blocking strategy scenarios.
* - An empty strategy is synonymous with blocking.
* - A script bundle (where $src is false) must always be blocking since the after inline script cannot be
* - A script alias (where $src is false) must always be blocking since the after inline script cannot be
* delayed as there is no external script tag and thus no load event at which the inline script can be run.
*/
if ( empty( $intended_strategy ) || empty( $this->registered[ $handle ]->src ) ) {
if ( empty( $intended_strategy ) ) {
return '';
}

// Aliases that have before/after inline scripts can never be delayed since there is no load event.
if ( ! $this->registered[ $handle ]->src && $this->has_inline_script( $handle ) ) {
return '';
}

Expand All @@ -1102,6 +1161,19 @@ private function get_eligible_loading_strategy( $handle ) {
return '';
}

/**
* Gets data for inline scripts registered for a specific handle.
*
* @since 6.3.0
*
* @param string $handle Name of the script to get data for.
* Must be lowercase.
* @return bool Whether the handle has an inline script (either before or after).
*/
private function has_inline_script( $handle ) {
return $this->get_data( $handle, 'before' ) || $this->get_data( $handle, 'after' );
}

/**
* Resets class properties.
*
Expand Down
100 changes: 90 additions & 10 deletions tests/phpunit/tests/dependencies/scripts.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,15 +633,15 @@ public function test_has_only_delayed_dependents( $set_up, $async_only, $expecte
}

/**
* Enqueue test script with before/after inline scripts.
* Register test script.
*
* @param string $handle Dependency handle to enqueue.
* @param string $strategy Strategy to use for dependency.
* @param string[] $deps Dependencies for the script.
* @param bool $in_footer Whether to print the script in the footer.
*/
protected function enqueue_test_script( $handle, $strategy, $deps = array(), $in_footer = false ) {
wp_enqueue_script(
protected function register_test_script( $handle, $strategy, $deps = array(), $in_footer = false ) {
wp_register_script(
$handle,
add_query_arg(
array(
Expand All @@ -657,6 +657,19 @@ protected function enqueue_test_script( $handle, $strategy, $deps = array(), $in
}
}

/**
* Enqueue test script.
*
* @param string $handle Dependency handle to enqueue.
* @param string $strategy Strategy to use for dependency.
* @param string[] $deps Dependencies for the script.
* @param bool $in_footer Whether to print the script in the footer.
*/
protected function enqueue_test_script( $handle, $strategy, $deps = array(), $in_footer = false ) {
$this->register_test_script( $handle, $strategy, $deps, $in_footer );
wp_enqueue_script( $handle );
}

/**
* Adds test inline script.
*
Expand Down Expand Up @@ -810,7 +823,6 @@ public function data_provider_to_test_various_strategy_dependency_chains() {
$handle1 = 'blocking-bundle-of-none';
$handle2 = 'defer-dependent-of-blocking-bundle-of-none';

// Note that jQuery is registered like this.
wp_register_script( $handle1, false, array(), null );
$this->add_test_inline_script( $handle1, 'before' );
$this->add_test_inline_script( $handle1, 'after' );
Expand All @@ -831,7 +843,7 @@ public function data_provider_to_test_various_strategy_dependency_chains() {
scriptEventLog.push( "defer-dependent-of-blocking-bundle-of-none: before inline" )
</script>
<script type='text/javascript' src='https://example.com/external.js?script_event_log=defer-dependent-of-blocking-bundle-of-none:%20script' id='defer-dependent-of-blocking-bundle-of-none-js' defer data-wp-strategy='defer'></script>
<script id="defer-dependent-of-blocking-bundle-of-none-js-after" type="text/plain" data-wp-deps="blocking-bundle-of-none">
<script id="defer-dependent-of-blocking-bundle-of-none-js-after" type="text/plain">
scriptEventLog.push( "defer-dependent-of-blocking-bundle-of-none: after inline" )
</script>
HTML
Expand All @@ -844,9 +856,9 @@ public function data_provider_to_test_various_strategy_dependency_chains() {
$handle3 = 'blocking-bundle-member-two';
$handle4 = 'defer-dependent-of-blocking-bundle-of-two';

wp_register_script( $handle1, false, array(), null );
$this->enqueue_test_script( $handle2, 'blocking', array( $handle1 ) );
$this->enqueue_test_script( $handle3, 'blocking', array( $handle1 ) );
wp_register_script( $handle1, false, array( $handle2, $handle3 ), null );
$this->enqueue_test_script( $handle2, 'blocking' );
$this->enqueue_test_script( $handle3, 'blocking' );
$this->enqueue_test_script( $handle4, 'defer', array( $handle1 ) );

foreach ( array( $handle2, $handle3, $handle4 ) as $handle ) {
Expand All @@ -873,7 +885,7 @@ public function data_provider_to_test_various_strategy_dependency_chains() {
scriptEventLog.push( "defer-dependent-of-blocking-bundle-of-two: before inline" )
</script>
<script type='text/javascript' src='https://example.com/external.js?script_event_log=defer-dependent-of-blocking-bundle-of-two:%20script' id='defer-dependent-of-blocking-bundle-of-two-js' defer data-wp-strategy='defer'></script>
<script id="defer-dependent-of-blocking-bundle-of-two-js-after" type="text/plain" data-wp-deps="blocking-bundle-of-two">
<script id="defer-dependent-of-blocking-bundle-of-two-js-after" type="text/plain" data-wp-deps="blocking-bundle-member-one,blocking-bundle-member-two">
scriptEventLog.push( "defer-dependent-of-blocking-bundle-of-two: after inline" )
</script>
HTML
Expand Down Expand Up @@ -906,7 +918,7 @@ public function data_provider_to_test_various_strategy_dependency_chains() {
scriptEventLog.push( "defer-dependent-of-defer-bundle-of-none: before inline" )
</script>
<script type='text/javascript' src='https://example.com/external.js?script_event_log=defer-dependent-of-defer-bundle-of-none:%20script' id='defer-dependent-of-defer-bundle-of-none-js' defer data-wp-strategy='defer'></script>
<script id="defer-dependent-of-defer-bundle-of-none-js-after" type="text/plain" data-wp-deps="defer-bundle-of-none">
<script id="defer-dependent-of-defer-bundle-of-none-js-after" type="text/plain">
scriptEventLog.push( "defer-dependent-of-defer-bundle-of-none: after inline" )
</script>
HTML
Expand Down Expand Up @@ -1046,6 +1058,74 @@ public function data_provider_to_test_various_strategy_dependency_chains() {
<script id="defer-with-after-inline-js-after" type="text/plain">
scriptEventLog.push( "defer-with-after-inline: after inline" )
</script>
HTML
,
),
'jquery-deferred' => array(
'set_up' => function () {
$wp_scripts = wp_scripts();
wp_default_scripts( $wp_scripts );
foreach ( $wp_scripts->registered['jquery']->deps as $jquery_dep ) {
$wp_scripts->registered[ $jquery_dep ]->add_data( 'strategy', 'defer' );
}
wp_enqueue_script( 'theme-functions', 'https://example.com/theme-functions.js', array( 'jquery' ), null, array( 'strategy' => 'defer' ) );
},
'expected_markup' => <<<HTML
<script type='text/javascript' src='http://example.org/wp-includes/js/jquery/jquery.js?ver=3.7.0' id='jquery-core-js' defer data-wp-strategy='defer'></script>
<script type='text/javascript' src='http://example.org/wp-includes/js/jquery/jquery-migrate.js?ver=3.4.0' id='jquery-migrate-js' defer data-wp-strategy='defer'></script>
<script type='text/javascript' src='https://example.com/theme-functions.js' id='theme-functions-js' defer data-wp-strategy='defer'></script>
HTML
,
),
'nested-aliases' => array(
'set_up' => function () {
$outer_alias_handle = 'outer-bundle-of-two';
$inner_alias_handle = 'inner-bundle-of-two';

// The outer alias contains a blocking member, as well as a nested alias that contains defer scripts.
wp_register_script( $outer_alias_handle, false, array( $inner_alias_handle, 'outer-bundle-leaf-member' ), null );
$this->register_test_script( 'outer-bundle-leaf-member', 'blocking', array() );

// Inner alias only contains delay scripts.
wp_register_script( $inner_alias_handle, false, array( 'inner-bundle-member-one', 'inner-bundle-member-two' ), null );
$this->register_test_script( 'inner-bundle-member-one', 'defer', array() );
$this->register_test_script( 'inner-bundle-member-two', 'defer', array() );

$this->enqueue_test_script( 'defer-dependent-of-nested-aliases', 'defer', array( $outer_alias_handle ) );
$this->add_test_inline_script( 'defer-dependent-of-nested-aliases', 'before' );
$this->add_test_inline_script( 'defer-dependent-of-nested-aliases', 'after' );
},
'expected_markup' => $this->get_delayed_inline_script_loader_script_tag() . <<<HTML
<script type='text/javascript' src='https://example.com/external.js?script_event_log=inner-bundle-member-one:%20script' id='inner-bundle-member-one-js' defer data-wp-strategy='defer'></script>
<script type='text/javascript' src='https://example.com/external.js?script_event_log=inner-bundle-member-two:%20script' id='inner-bundle-member-two-js' defer data-wp-strategy='defer'></script>
<script type='text/javascript' src='https://example.com/external.js?script_event_log=outer-bundle-leaf-member:%20script' id='outer-bundle-leaf-member-js'></script>
<script id="defer-dependent-of-nested-aliases-js-before" type="text/plain" data-wp-deps="inner-bundle-member-one,inner-bundle-member-two,outer-bundle-leaf-member">
scriptEventLog.push( "defer-dependent-of-nested-aliases: before inline" )
</script>
<script type='text/javascript' src='https://example.com/external.js?script_event_log=defer-dependent-of-nested-aliases:%20script' id='defer-dependent-of-nested-aliases-js' defer data-wp-strategy='defer'></script>
<script id="defer-dependent-of-nested-aliases-js-after" type="text/plain" data-wp-deps="inner-bundle-member-one,inner-bundle-member-two,outer-bundle-leaf-member">
scriptEventLog.push( "defer-dependent-of-nested-aliases: after inline" )
</script>
HTML
,
),

'async-alias-members-with-defer-dependency' => array(
'set_up' => function () {
$alias_handle = 'async-alias';
$async_handle1 = 'async1';
$async_handle2 = 'async2';

wp_register_script( $alias_handle, false, array( $async_handle1, $async_handle2 ), null );
$this->register_test_script( $async_handle1, 'async', array() );
$this->register_test_script( $async_handle2, 'async', array() );

$this->enqueue_test_script( 'defer-dependent-of-async-aliases', 'defer', array( $alias_handle ) );
},
'expected_markup' => <<<HTML
<script type='text/javascript' src='https://example.com/external.js?script_event_log=async1:%20script' id='async1-js' defer data-wp-strategy='async'></script>
<script type='text/javascript' src='https://example.com/external.js?script_event_log=async2:%20script' id='async2-js' defer data-wp-strategy='async'></script>
<script type='text/javascript' src='https://example.com/external.js?script_event_log=defer-dependent-of-async-aliases:%20script' id='defer-dependent-of-async-aliases-js' defer data-wp-strategy='defer'></script>
HTML
,
),
Expand Down