Add / 48 Abort Script Strategy Logic When Concat Scripts Present #51

10upsimon · 2023-03-27T11:36:32Z

Fixes #48

Aborts script concatenation per handle and resets $this->do_concat if the handle in question is of a defer or async loading strategy.

Trac ticket:

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.

…fer or async loading strategy.

adamsilverstein · 2023-03-27T18:21:39Z

src/wp-includes/class-wp-scripts.php

+			// Get the most eligible loading strategy for said script handle.
+			// Used as a conditional to prevent script concatenation.
+			$strategy                    = $this->get_eligible_loading_strategy( $handle );
+			$is_deferred_or_async_handle = '' !== $strategy;


maybe check explicitly for 'async' or defer' here since other strategies could be added in the future.

@adamsilverstein this has been resolved in a recent commit.

joemcgill

I think this looks right but it would be good to confirm this expected behavior with a unit test or two. Is that something you can add?

…abort-script-strategy-logic-when-concat-scripts-present

…xiting

10upsimon · 2023-03-29T08:25:35Z

I think this looks right but it would be good to confirm this expected behavior with a unit test or two. Is that something you can add?

@joemcgill this was addressed in a recent commit by @kt-12

joemcgill

This is looking good. A couple of suggestions, but it's nice to see this working as expected. Nice work!

joemcgill · 2023-03-29T14:38:19Z

tests/phpunit/tests/dependencies/scripts.php

+		wp_enqueue_script( 'main-defer-script-1', '/main-script.js', array(), null, array( 'strategy' => 'async' ) );
+
+		wp_print_scripts();
+		$print_scripts = get_echo( '_print_scripts' );
+
+		// reset global before asserting.
+		$concatenate_scripts = $old_value;
+
+		$ver       = get_bloginfo( 'version' );
+		$expected  = "<script type='text/javascript' src='/wp-admin/load-scripts.php?c=0&amp;load%5Bchunk_0%5D=one-concat-dep-1,two-concat-dep-1,three-concat-dep-1&amp;ver={$ver}'></script>\n";
+		$expected .= "<script type='text/javascript' src='/main-script.js' id='main-defer-script-1-js' async></script>\n";


Update the handle name so it matches the loading strategy.

Suggested change

wp_enqueue_script( 'main-defer-script-1', '/main-script.js', array(), null, array( 'strategy' => 'async' ) );

wp_print_scripts();

$print_scripts = get_echo( '_print_scripts' );

// reset global before asserting.

$concatenate_scripts = $old_value;

$ver = get_bloginfo( 'version' );

$expected = "<script type='text/javascript' src='/wp-admin/load-scripts.php?c=0&load%5Bchunk_0%5D=one-concat-dep-1,two-concat-dep-1,three-concat-dep-1&ver={$ver}'></script>\n";

$expected .= "<script type='text/javascript' src='/main-script.js' id='main-defer-script-1-js' async></script>\n";

wp_enqueue_script( 'main-async-script-1', '/main-script.js', array(), null, array( 'strategy' => 'async' ) );

wp_print_scripts();

$print_scripts = get_echo( '_print_scripts' );

// reset global before asserting.

$concatenate_scripts = $old_value;

$ver = get_bloginfo( 'version' );

$expected = "<script type='text/javascript' src='/wp-admin/load-scripts.php?c=0&load%5Bchunk_0%5D=one-concat-dep-1,two-concat-dep-1,three-concat-dep-1&ver={$ver}'></script>\n";

$expected .= "<script type='text/javascript' src='/main-script.js' id='main-async-script-1-js' async></script>\n";

joemcgill · 2023-03-29T14:41:30Z

tests/phpunit/tests/dependencies/scripts.php

+		$wp_scripts->do_concat    = true;
+		$wp_scripts->default_dirs = array( '/directory/' );
+
+		wp_enqueue_script( 'one-concat-dep-2', '/directory/script.js' );


This makes me curious about what we would expect to happen if there's a mix of concatenated scripts that depend on a deferred script or if only some of these concatenated scripts were dependencies of the deferred script.

If any script depends on a deferred script, the deferred script itself will be loaded in a blocking way. In this case, it will get concatenated.get_strategy takes care of that.

joemcgill · 2023-03-29T14:43:57Z

tests/phpunit/tests/dependencies/scripts.php

+	public function test_concatenate_with_async_strategy() {
+		global $wp_scripts, $concatenate_scripts;
+
+		$old_value = $concatenate_scripts;


Since we are modifying the global $wp_scripts variable directly, I wonder if we even need to modify the global $concatenate_scripts variable? We may also need to reset the global $wp_scripts variable before running or assertions.

@joemcgill This is the case with almost every test case. Infact, most of the test case contains handles from previous test cases. I followed this rule ->
as long as this doesn't affect other test cases, not resetting global should be fine.

This function was created by replicating an old concatenation test function, setting of the directory was also taken from it.

cc: @10upsimon.

Thanks for the context Karthik. I’ll make a note to review our use of that global in all the tests. Not something we need to address here.

joemcgill · 2023-03-29T14:45:09Z

tests/phpunit/tests/dependencies/scripts.php

+		$concatenate_scripts = true;
+
+		$wp_scripts->do_concat    = true;
+		$wp_scripts->default_dirs = array( '/directory/' );


Nitpick: we could make the default dir path a variable so it can easily be updated everywhere.

@joemcgill solved in a recent commit.

…ripts-present' of github.com:10up/wordpress-develop into add/issue-48-abort-script-strategy-logic-when-concat-scripts-present

…nstances.

…instead reference new protected property.

joemcgill

Thanks @kt-12 and @10upsimon for getting tests added and fixing up the formatting issue that we discovered in the process of debugging. I went ahead and updated the handle name after confirming in Slack that you were both ok with this change, and also fixed an additional CS issue with spacing that I discovered locally. This is good to merge from my view.

kt-12

Thank you, @10upsimon, Looks good to me!

…abort-script-strategy-logic-when-concat-scripts-present

Abort/reset script concatenation if the handle in question is of a de…

24452a6

…fer or async loading strategy.

10upsimon added needs:code-review This requires code review. type:enhancement New feature or request. Script Loading Strategy Issues relating to the Script Loading Strategy (WPP) labels Mar 27, 2023

10upsimon requested review from joemcgill, felixarntz and kt-12 March 27, 2023 11:36

10upsimon self-assigned this Mar 27, 2023

Fixed comment typo

08f200f

adamsilverstein reviewed Mar 27, 2023

View reviewed changes

joemcgill linked an issue Mar 27, 2023 that may be closed by this pull request

Abort script strategy logical checks when CONCATENATE_SCRIPTS is true #48

Closed

1 task

joemcgill reviewed Mar 27, 2023

View reviewed changes

Merge branch 'enhancement/implement_load_handlers' into add/issue-48-…

f9355cd

…abort-script-strategy-logic-when-concat-scripts-present

kt-12 changed the base branch from enhancement/wp-script-api-strategy-base to enhancement/implement_load_handlers March 28, 2023 15:22

kt-12 and others added 4 commits March 28, 2023 17:00

strategy already available

ae38432

Add more specific defer and async strategy checks for concatenation e…

54c74be

…xiting

test defer script with concat

31e4507

concat with async

2031aff

blocking before and after defer

6beabce

joemcgill reviewed Mar 29, 2023

View reviewed changes

10upsimon and others added 6 commits March 29, 2023 21:47

Merge origin into local

a8dba9f

Merge branch 'add/issue-48-abort-script-strategy-logic-when-concat-sc…

6d75add

…ripts-present' of github.com:10up/wordpress-develop into add/issue-48-abort-script-strategy-logic-when-concat-scripts-present

Remove two trailing spaces in printed output that were failing test i…

ffe725c

…nstances.

Replaced hard coded references to default script directory string to …

c656495

…instead reference new protected property.

Fix spacing for PHPCS

23630b9

Update script handle name to match async strategy in tests

2087305

joemcgill approved these changes Mar 30, 2023

View reviewed changes

kt-12 reviewed Mar 30, 2023

View reviewed changes

Merge branch 'enhancement/implement_load_handlers' into add/issue-48-…

fdf0bd3

…abort-script-strategy-logic-when-concat-scripts-present

kt-12 added 2 commits March 31, 2023 13:36

remove extra variable

4b16162

remove hardcoded type

0c7d18e

Base automatically changed from enhancement/implement_load_handlers to enhancement/add_text_template April 3, 2023 05:48

Base automatically changed from enhancement/add_text_template to enhancement/wp-script-api-strategy-base April 3, 2023 05:50

Merge branch 'enhancement/implement_load_handlers' into add/issue-48-…

f61d867

…abort-script-strategy-logic-when-concat-scripts-present

kt-12 merged commit 90e6105 into enhancement/wp-script-api-strategy-base Apr 3, 2023

kt-12 deleted the add/issue-48-abort-script-strategy-logic-when-concat-scripts-present branch April 3, 2023 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add / 48 Abort Script Strategy Logic When Concat Scripts Present #51

Add / 48 Abort Script Strategy Logic When Concat Scripts Present #51

10upsimon commented Mar 27, 2023

adamsilverstein Mar 27, 2023

10upsimon Mar 29, 2023

joemcgill left a comment

10upsimon commented Mar 29, 2023

joemcgill left a comment

joemcgill Mar 29, 2023

joemcgill Mar 29, 2023

kt-12 Mar 29, 2023 •

edited

Loading

joemcgill Mar 29, 2023

kt-12 Mar 29, 2023

joemcgill Mar 29, 2023

joemcgill Mar 29, 2023

10upsimon Mar 30, 2023

joemcgill left a comment

kt-12 left a comment

Add / 48 Abort Script Strategy Logic When Concat Scripts Present #51

Add / 48 Abort Script Strategy Logic When Concat Scripts Present #51

Conversation

10upsimon commented Mar 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

10upsimon commented Mar 29, 2023

joemcgill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kt-12 Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

kt-12 left a comment

Choose a reason for hiding this comment

kt-12 Mar 29, 2023 •

edited

Loading