Implement load handlers #50

kt-12 · 2023-03-10T22:10:04Z

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.

10upsimon · 2023-03-13T09:09:21Z

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

+		$output = <<<JS
+let wpLoadAfterScripts = ( handle ) => {
+	let scripts = document.querySelectorAll(`[type="text/template"][data-wp-executes-after="\${handle}"]`);
+	scripts.forEach( (script) => { script.setAttribute("type","text/javascript") })


@kt-12 is this enough to get the browser to actually execute the script by just converting its type? I'm quite sure that during the discovery phase we has ascertained that this may need manual execution and that the browser would not automatically fire the JS tag just because its type was dynamically altered.

Can you confirm this please?

@kt-12 upon further research, it seems to me that changing the type attribute alone will not execute the JS, and even inline injection/alteration would not. We're likely going to have to use appendChild() or a similar method, where we clone the text/template tag, change the attribute, find the DOM element that exists currently (i.e the text/template tag in question) and append the new JS tag after it. We would probably want to then remove the text/template element.

A series of events may look like this:

Iterate over all text/template tags as you currently do above

Reference that DOM element

Reference the full tag as text form, as a variable

Change the type to text/javascript as you're doing above

Use appendChild() to append the new script string after the DOM element referenced in point 2

Consider deleting the DOM element referenced in point 2 above, but you'd need to make a call if this has implications and whether we should keep it.

10upsimon · 2023-03-13T09:23:06Z

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

+		$output = <<<JS
+let wpLoadAfterScripts = ( handle ) => {
+	let scripts = document.querySelectorAll(`[type="text/template"][data-wp-executes-after="\${handle}"]`);
+	scripts.forEach( (script) => { script.setAttribute("type","text/javascript") })


@kt-12 upon further research, it seems to me that changing the type attribute alone will not execute the JS, and even inline injection/alteration would not. We're likely going to have to use appendChild() or a similar method, where we clone the text/template tag, change the attribute, find the DOM element that exists currently (i.e the text/template tag in question) and append the new JS tag after it. We would probably want to then remove the text/template element.

A series of events may look like this:

Iterate over all text/template tags as you currently do above

Reference that DOM element

Reference the full tag as text form, as a variable

Change the type to text/javascript as you're doing above

Use appendChild() to append the new script string after the DOM element referenced in point 2

Consider deleting the DOM element referenced in point 2 above, but you'd need to make a call if this has implications and whether we should keep it.

10upsimon

@kt-12 minor observations, comments and small change requests noted.

10upsimon · 2023-03-21T11:04:10Z

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

+	 *
+	 * @return bool True if script present. False if empty.
+	 */
+	public function maybe_has_delayed_inline_script() {


@kt-12 this is a nitpick comment I guess, but I am wondering if maybe is the correct wording here, as it seems to be like you are indefinitely checking of a deferred or async script has inline scripts? This is not a case of maybe and seems like it's more a definite outcome?

If you agree, let's rename this to has_delayed_inline_script() as that is more true to the logic here.

If I've misunderstood, please comment as such thank you :)

You are correct. I used maybe as there are certain cases where the final strategy of a script might change before we print them effecting it's inline scripts decision. For example a defer may change to blocking, but we never know it in print_scripts action.

But i do agree maybe is not needed here, i'll remove that.

tests/phpunit/tests/dependencies/scripts.php

Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>

…com/10up/wordpress-develop into enhancement/implement_load_handlers

…nt_load_handlers

mukeshpanchal27

Thanks @kt-12, I quick review and left some feedback and questions.

mukeshpanchal27 · 2023-03-27T10:35:41Z

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

+					'<script%1$s id=\'%2$s-js-after\' type=\'text/template\' data-wp-executes-after=\'%2$s\'>%4$s%3$s%4$s</script>%4$s',
+					$this->type_attr,
+					esc_attr( $handle ),
+					$after_non_standalone_handle,
+					PHP_EOL


Suggested change

'<script%1$s id=\'%2$s-js-after\' type=\'text/template\' data-wp-executes-after=\'%2$s\'>%4$s%3$s%4$s</script>%4$s',

$this->type_attr,

esc_attr( $handle ),

$after_non_standalone_handle,

PHP_EOL

'<script%s id=\'%2$s-js-after\' type=\'text/template\' data-wp-executes-after=\'%s\'>\n%s\n</script>\n',

$this->type_attr,

esc_attr( $handle ),

$after_non_standalone_handle,

Similar to other scripts, use \n instead of PHP_EOL and use %s as it is sufficient for these changes as we don't need to add translators or change the order of the arguments.

@mukeshpanchal27 I was trying the same initially but \n don't work when the parent string is in single quotes. It is instead printed as string \n. Numbered placeholder was added as there are multiple places i wanted the handle string and for numbered placeholder I need the enclosing string to be in single quotes.

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

mukeshpanchal27 · 2023-03-27T10:39:04Z

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

@@ -771,6 +788,23 @@ public function add_data( $handle, $key, $value ) {
 		return parent::add_data( $handle, $key, $value );
 	}

+	/**
+	 * Check all handles for any delayed inline scripts.
+	 *


@Since annotation is missing.

@mukeshpanchal27 We haven't fixed on the final @since yet. We have a separate task for this.

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

mukeshpanchal27 · 2023-03-27T10:50:37Z

src/wp-includes/script-loader.php

+	})
+}
+JS;
+		printf( "<script id='wp-executes-after-js'>\n%s\n</script>\n", $output );


$this->type_attr is missing in script.

Fixed, but $this->type_attr is not available here so replicate the way function _print_scripts() was handling the same scenario.

mukeshpanchal27 · 2023-03-27T10:52:50Z

src/wp-includes/script-loader.php

+/**
+ * Prints a loader script if there is text/template registered script.
+ */


Per PHP Documentation Standards, Use proper format for function

Updated. Usage of since is still missing as this will be handled together for the entire functionality in a separate task. Thanks @mukeshpanchal27.

mukeshpanchal27 · 2023-03-27T10:54:24Z

src/wp-includes/script-loader.php

+let wpLoadAfterScripts = ( handle ) => {
+	let scripts = document.querySelectorAll(`[type="text/template"][data-wp-executes-after="\${handle}"]`);
+	scripts.forEach( (script) => { 
+		script.setAttribute("type","text/javascript"); 


Suggested change

script.setAttribute("type","text/javascript");

WP use $this->type_attr for script type. Please correct me if i miss anything.

@mukeshpanchal27 this is the JS code for modifying type after a script is loaded on the DOM. So we can't use $this->type_attr

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

…nt_load_handlers

joemcgill

A couple small nits, but this seems ok to me. Pre-approving since nothing is blocking.

joemcgill · 2023-03-31T12:48:24Z

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

+			if ( in_array( $this->get_intended_strategy( $handle ), array( 'defer', 'async' ), true ) ) {
+				// non standalone after scripts of async or defer are usually delayed.
+				if ( $this->has_non_standalone_inline_script( $handle, 'after' ) ) {


These could be combined into a single if statement with &&

joemcgill · 2023-03-31T12:51:26Z

src/wp-includes/script-loader.php

+	})
+}
+JS;
+		$type_attr = current_theme_supports( 'html5', 'script' ) ? '' : " type='text/javascript'";


This could probably use $wp_scripts->type_attr since it's the same functionality, right?

@joemcgill we can't use it here as, type_attr is a private attribute.

joemcgill · 2023-03-31T13:06:15Z

src/wp-includes/script-loader.php

+	$wp_scripts = wp_scripts();
+	if ( $wp_scripts->has_delayed_inline_script() ) {
+		$output    = <<<JS
+let wpLoadAfterScripts = ( handle ) => {


This could probably be a regular function rather than an arrow function.

function wpLoadAfterScripts( handle ) { // functionality }

joemcgill · 2023-03-31T13:50:24Z

src/wp-includes/script-loader.php

 * associated with a handle to type/javascript and execute them.
 */
 function wp_print_template_loader_script() {
 	$wp_scripts = wp_scripts();
 	if ( $wp_scripts->has_delayed_inline_script() ) {
 		$output    = <<<JS
-let wpLoadAfterScripts = ( handle ) => {
+function wpLoadAfterScripts() {


You still need to accept a handle param, right?

joemcgill · 2023-03-31T14:03:54Z

Thanks @kt-12. I've already approved so this is good to merge whenever you're ready.

kt-12 added 7 commits March 10, 2023 20:35

numbered attributes

04aabf5

onload script

e366d5d

fix bug

0fafd88

sprintf issue with \n

386022e

phpcs fixes

fe7acc0

print template loader script

394405f

\n to EOL

cc5dace

kt-12 added this to the Milestone 3 milestone Mar 11, 2023

kt-12 linked an issue Mar 11, 2023 that may be closed by this pull request

Implementation of load handlers to ensure that the execution order of inline scripts is retained, where applicable #20

Closed

1 task

10upsimon approved these changes Mar 13, 2023

View reviewed changes

10upsimon self-requested a review March 13, 2023 09:07

10upsimon reviewed Mar 13, 2023

View reviewed changes

10upsimon requested changes Mar 13, 2023

View reviewed changes

kt-12 added 2 commits March 13, 2023 15:52

Loading the script before other script

b558f34

temp commit

f04a246

kt-12 added the Script Loading Strategy Issues relating to the Script Loading Strategy (WPP) label Mar 14, 2023

check if any delayed inline scripts

b5f4df4

kt-12 self-assigned this Mar 15, 2023

kt-12 added 6 commits March 15, 2023 20:06

Merge add_text_template to implement_load_handlers

d9a0f39

add new line

d110556

backward compatibility

b6b7022

update test utility and scripts.

9e8c378

php cs fixes

ffc40c1

fix csfix issue

8af918c

10upsimon requested changes Mar 21, 2023

View reviewed changes

kt-12 and others added 5 commits March 21, 2023 11:18

Update test message.

d952da5

Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>

Update test comment

b71b557

Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>

Update test error message

479d656

Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>

change function names

968d5b7

Merge branch 'enhancement/implement_load_handlers' of https://github.…

ad9d9c3

…com/10up/wordpress-develop into enhancement/implement_load_handlers

kt-12 added 5 commits March 22, 2023 10:16

use unregister column

326555d

Merge branch 'enhancement/add_text_template' into enhancement/impleme…

6187dd1

…nt_load_handlers

avoid removal of jquery libraries

1321d6f

Dataprovider doc block.

66ab60c

doc block change

bbdad37

10upsimon approved these changes Mar 27, 2023

View reviewed changes

mukeshpanchal27 reviewed Mar 27, 2023

View reviewed changes

kt-12 and others added 3 commits March 28, 2023 14:24

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

966be0c

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

Update doc text

dcbf0af

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

Merge branch 'enhancement/add_text_template' into enhancement/impleme…

8e08454

…nt_load_handlers

This was referenced Mar 31, 2023

Add text template #49

Merged

Script Loading Strategy - Update standalone business logic #47

Merged

kt-12 added 6 commits March 31, 2023 11:09

fix type_attr no there issue

b3f7225

adding description

4df5288

broke standalone script test to multiple function

264853c

non-standalone after script test broken

556ae91

before standalone test broken

d4be88d

non standalone with before and defer

180a6ae

kt-12 requested a review from joemcgill March 31, 2023 12:34

joemcgill approved these changes Mar 31, 2023

View reviewed changes

joemcgill reviewed Mar 31, 2023

View reviewed changes

kt-12 added 2 commits March 31, 2023 14:34

used function instead of arrow function.

4d0140d

single condition

a5b83a4

joemcgill reviewed Mar 31, 2023

View reviewed changes

put back handle

0269128

10upsimon merged commit 4cc2502 into enhancement/add_text_template Apr 3, 2023

10upsimon deleted the enhancement/implement_load_handlers branch April 3, 2023 05:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement load handlers #50

Implement load handlers #50

kt-12 commented Mar 10, 2023

10upsimon Mar 13, 2023

10upsimon Mar 13, 2023

10upsimon Mar 13, 2023

10upsimon left a comment

10upsimon Mar 21, 2023

kt-12 Mar 21, 2023

mukeshpanchal27 left a comment

mukeshpanchal27 Mar 27, 2023

kt-12 Mar 31, 2023

mukeshpanchal27 Mar 27, 2023

kt-12 Mar 28, 2023

mukeshpanchal27 Mar 27, 2023

kt-12 Mar 31, 2023

mukeshpanchal27 Mar 27, 2023

kt-12 Mar 31, 2023

mukeshpanchal27 Mar 27, 2023

kt-12 Mar 28, 2023

joemcgill left a comment

joemcgill Mar 31, 2023

joemcgill Mar 31, 2023

kt-12 Mar 31, 2023

joemcgill Mar 31, 2023

joemcgill Mar 31, 2023

kt-12 Mar 31, 2023

joemcgill commented Mar 31, 2023

Implement load handlers #50

Implement load handlers #50

Conversation

kt-12 commented Mar 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

10upsimon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mukeshpanchal27 left a 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

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

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 commented Mar 31, 2023