Skip to content
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

Script Loading Strategy - Update standalone business logic #47

Merged
merged 21 commits into from
Apr 1, 2023
Merged

Script Loading Strategy - Update standalone business logic #47

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
043c244
Handling inline before
kt-12 Mar 6, 2023
53a54e6
TODO Note for on Load logic for after
kt-12 Mar 6, 2023
d90ed89
Before Logic.
kt-12 Mar 9, 2023
514876f
test cases
kt-12 Mar 9, 2023
2c31104
bug fix
kt-12 Mar 9, 2023
4e1aaaf
Add non standalone test cases.
kt-12 Mar 9, 2023
df906be
stand alone test case
kt-12 Mar 10, 2023
484789f
Merge branch 'enhancement/wp-add-inline-script-standalone' into enhan…
kt-12 Mar 10, 2023
3ad953f
keep a track of standalone and non-standalone
kt-12 Mar 10, 2023
2eb8aed
Update src/wp-includes/class-wp-scripts.php
kt-12 Mar 15, 2023
07179c8
update comment text
kt-12 Mar 22, 2023
169fc28
Update comment
kt-12 Mar 22, 2023
ad9cafb
Update comment
kt-12 Mar 22, 2023
5cda63d
Update comment
kt-12 Mar 22, 2023
a587e5e
Update doc block.
kt-12 Mar 22, 2023
b861844
Update doc block
kt-12 Mar 22, 2023
70122ff
Update comment
kt-12 Mar 22, 2023
c81fdf2
Update comment
kt-12 Mar 22, 2023
2dce5f4
Update comment
kt-12 Mar 22, 2023
2571691
Update comment
kt-12 Mar 22, 2023
b7ac63f
grammar fix
kt-12 Mar 22, 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
27 changes: 27 additions & 0 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 @@ -402,6 +402,7 @@ public function do_item( $handle, $group = false ) {
esc_attr( $handle ),
$strategy
);
// TODO: Handle onload logic for defer/async here.
10upsimon marked this conversation as resolved.
Show resolved Hide resolved
$tag .= $after_handle . $cond_after;

/**
Expand Down Expand Up @@ -449,6 +450,12 @@ public function add_inline_script( $handle, $data, $position = 'after', $standal
$script = (array) $this->get_data( $handle, $position );
$script[] = $data;

// Keep a list of standalone and non-standalone before/after scripts.
kt-12 marked this conversation as resolved.
Show resolved Hide resolved
$standalone_key = $standalone ? $position . '-standalone' : $position . '-non-standalone';
$standalone_script = (array) $this->get_data( $handle, $standalone_key );
$standalone_script[] = $data;
$this->add_data( $handle, $standalone_key, $standalone_script );

return $this->add_data( $handle, $position, $script );
}

Expand Down Expand Up @@ -797,6 +804,20 @@ private function get_intended_strategy( $handle ) {
return isset( $script_args['strategy'] ) ? $script_args['strategy'] : false;
}

/**
* Get the strategy assigned during script registration.
*
* @param string $handle The script handle.
* @param string $position Position of the inline script.
kt-12 marked this conversation as resolved.
Show resolved Hide resolved
*
* @return bool True if script present. False if empty.
*/
private function has_non_standalone_inline_script( $handle, $position ) {
$non_standalone_script_key = $position . '-non-standalone';
$non_standalone_script = $this->get_data( $handle, $non_standalone_script_key );
return ! empty( $non_standalone_script );
}

/**
* Check if all of a scripts dependents are deferrable which is required to maintain execution order.
kt-12 marked this conversation as resolved.
Show resolved Hide resolved
*
Expand Down Expand Up @@ -824,11 +845,17 @@ private function all_dependents_are_deferrable( $handle, $checked = array() ) {
return false;
}

// If the dependent script has a before then non-standalone 'before' inline script then not defer.
kt-12 marked this conversation as resolved.
Show resolved Hide resolved
if ( $this->has_non_standalone_inline_script( $dependent, 'before' ) ) {
return false;
}

// Recursively check all dependents.
if ( ! $this->all_dependents_are_deferrable( $dependent, $checked ) ) {
return false;
}
}

return true;
}

Expand Down
128 changes: 128 additions & 0 deletions tests/phpunit/tests/dependencies/scripts.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,134 @@ public function test_wp_enqueue_script() {
$this->assertSame( '', get_echo( 'wp_print_scripts' ) );
}

/**
* Test non standalone before with defer.
*
* @ticket 12009
* @dataProvider data_non_standalone_before_inline_script_with_defer
*/
kt-12 marked this conversation as resolved.
Show resolved Hide resolved
public function test_non_standalone_before_inline_script_with_defer( $expected, $output, $message ) {
$this->assertSame( $expected, $output, $message );
}

public function data_non_standalone_before_inline_script_with_defer() {
Copy link

Choose a reason for hiding this comment

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

@kt-12 missing inline doc block.

Copy link
Author

Choose a reason for hiding this comment

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

I don't see a separate doc block for dataProvider in most of the existing test cases. e.g data_application_passwords_can_use_capability_checks_to_determine_feature_availability in /tests/phpunit/tests/auth.php.

But, there are also functions that have added doc block e.g data_admin_bar_nodes_with_tabindex_meta in /tests/phpunit/tests/adminbar.php

Copy link
Collaborator

Choose a reason for hiding this comment

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

Generally speaking, data providers are meant to provide inputs into whatever functionality you are unit testing, and not just passing parameters to a PHPUnit assertion. I think that these would probably be better broken up into several individual tests instead.

Copy link
Author

Choose a reason for hiding this comment

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

@joemcgill @10upsimon. To avoid re-working on the same test multiple time. I'll handle break down test cases in ->

PR #50 - enhancement/implement_load_handlers

$data = array();

// If the main script has a `before` inline script; all dependencies will be blocking.
kt-12 marked this conversation as resolved.
Show resolved Hide resolved
wp_enqueue_script( 'ds-i1-1', 'http://example.org/ds-i1-1.js', array(), null, array( 'strategy' => 'defer' ) );
wp_enqueue_script( 'ds-i1-2', 'http://example.org/ds-i1-2.js', array(), null, array( 'strategy' => 'defer' ) );
wp_enqueue_script( 'ds-i1-3', 'http://example.org/ds-i1-3.js', array(), null, array( 'strategy' => 'defer' ) );
wp_enqueue_script( 'ms-i1-1', 'http://example.org/ms-i1-1.js', array( 'ds-i1-1', 'ds-i1-2', 'ds-i1-3' ), null, array( 'strategy' => 'defer' ) );
wp_add_inline_script( 'ms-i1-1', 'console.log("before one");', 'before' );
$output = get_echo( 'wp_print_scripts' );
$expected = "<script type='text/javascript' src='http://example.org/ds-i1-1.js' id='ds-i1-1-js'></script>\n";
$expected .= "<script type='text/javascript' src='http://example.org/ds-i1-2.js' id='ds-i1-2-js'></script>\n";
$expected .= "<script type='text/javascript' src='http://example.org/ds-i1-3.js' id='ds-i1-3-js'></script>\n";
$expected .= "<script type='text/javascript' id='ms-i1-1-js-before'>\n";
$expected .= "console.log(\"before one\");\n";
$expected .= "</script>\n";
$expected .= "<script type='text/javascript' src='http://example.org/ms-i1-1.js' id='ms-i1-1-js' defer></script>\n";
array_push( $data, array( $expected, $output, 'All dependency in the chain should be blocking' ) );

// One of the dependency in the chain has a `before` inline script; all script above it will be blocking.
kt-12 marked this conversation as resolved.
Show resolved Hide resolved
wp_enqueue_script( 'ds-i2-1', 'http://example.org/ds-i2-1.js', array(), null, array( 'strategy' => 'defer' ) );
wp_enqueue_script( 'ds-i2-2', 'http://example.org/ds-i2-2.js', array( 'ds-i2-1' ), null, array( 'strategy' => 'defer' ) );
wp_enqueue_script( 'ds-i2-3', 'http://example.org/ds-i2-3.js', array( 'ds-i2-2' ), null, array( 'strategy' => 'defer' ) );
wp_enqueue_script( 'ms-i2-1', 'http://example.org/ms-i2-1.js', array( 'ds-i2-3' ), null, array( 'strategy' => 'defer' ) );
wp_add_inline_script( 'ds-i2-2', 'console.log("before one");', 'before' );
$output = get_echo( 'wp_print_scripts' );
$expected = "<script type='text/javascript' src='http://example.org/ds-i2-1.js' id='ds-i2-1-js'></script>\n";
$expected .= "<script type='text/javascript' id='ds-i2-2-js-before'>\n";
$expected .= "console.log(\"before one\");\n";
$expected .= "</script>\n";
$expected .= "<script type='text/javascript' src='http://example.org/ds-i2-2.js' id='ds-i2-2-js' defer></script>\n";
$expected .= "<script type='text/javascript' src='http://example.org/ds-i2-3.js' id='ds-i2-3-js' defer></script>\n";
$expected .= "<script type='text/javascript' src='http://example.org/ms-i2-1.js' id='ms-i2-1-js' defer></script>\n";
array_push( $data, array( $expected, $output, 'Scripts in the chain before the script having before must be blocking.' ) );

// Top most dependency in the chain has a `before` inline script; none of the script bellow it will be blocking.
kt-12 marked this conversation as resolved.
Show resolved Hide resolved
wp_enqueue_script( 'ds-i3-1', 'http://example.org/ds-i3-1.js', array(), null, array( 'strategy' => 'defer' ) );
wp_enqueue_script( 'ds-i3-2', 'http://example.org/ds-i3-2.js', array( 'ds-i3-1' ), null, array( 'strategy' => 'defer' ) );
wp_enqueue_script( 'ms-i3-1', 'http://example.org/ms-i3-1.js', array( 'ds-i3-2' ), null, array( 'strategy' => 'defer' ) );
wp_add_inline_script( 'ds-i3-1', 'console.log("before one");', 'before' );
$output = get_echo( 'wp_print_scripts' );
$expected = "<script type='text/javascript' id='ds-i3-1-js-before'>\n";
$expected .= "console.log(\"before one\");\n";
$expected .= "</script>\n";
$expected .= "<script type='text/javascript' src='http://example.org/ds-i3-1.js' id='ds-i3-1-js' defer></script>\n";
$expected .= "<script type='text/javascript' src='http://example.org/ds-i3-2.js' id='ds-i3-2-js' defer></script>\n";
$expected .= "<script type='text/javascript' src='http://example.org/ms-i3-1.js' id='ms-i3-1-js' defer></script>\n";
array_push( $data, array( $expected, $output, 'Top most has before inline script. All the script in the chain defer.' ) );

// If there are two dependencies chain; rules are applied to the scripts in the chain having a `before` inline script.
kt-12 marked this conversation as resolved.
Show resolved Hide resolved
wp_enqueue_script( 'ch1-ds-i4-1', 'http://example.org/ch1-ds-i4-1.js', array(), null, array( 'strategy' => 'defer' ) );
wp_enqueue_script( 'ch1-ds-i4-2', 'http://example.org/ch1-ds-i4-2.js', array( 'ch1-ds-i4-1' ), null, array( 'strategy' => 'defer' ) );
wp_enqueue_script( 'ch2-ds-i4-1', 'http://example.org/ch2-ds-i4-1.js', array(), null, array( 'strategy' => 'defer' ) );
wp_enqueue_script( 'ch2-ds-i4-2', 'http://example.org/ch2-ds-i4-2.js', array( 'ch2-ds-i4-1' ), null, array( 'strategy' => 'defer' ) );
wp_add_inline_script( 'ch2-ds-i4-2', 'console.log("before one");', 'before' );
wp_enqueue_script( 'ms-i4-1', 'http://example.org/ms-i4-1.js', array( 'ch2-ds-i4-1', 'ch2-ds-i4-2' ), null, array( 'strategy' => 'defer' ) );
$output = get_echo( 'wp_print_scripts' );
$expected = "<script type='text/javascript' src='http://example.org/ch1-ds-i4-1.js' id='ch1-ds-i4-1-js' defer></script>\n";
$expected .= "<script type='text/javascript' src='http://example.org/ch1-ds-i4-2.js' id='ch1-ds-i4-2-js' defer></script>\n";
$expected .= "<script type='text/javascript' src='http://example.org/ch2-ds-i4-1.js' id='ch2-ds-i4-1-js'></script>\n";
$expected .= "<script type='text/javascript' id='ch2-ds-i4-2-js-before'>\n";
$expected .= "console.log(\"before one\");\n";
$expected .= "</script>\n";
$expected .= "<script type='text/javascript' src='http://example.org/ch2-ds-i4-2.js' id='ch2-ds-i4-2-js' defer></script>\n";
$expected .= "<script type='text/javascript' src='http://example.org/ms-i4-1.js' id='ms-i4-1-js' defer></script>\n";
array_push( $data, array( $expected, $output, 'Only top dependency script in chain two should be blocking.' ) );

return $data;
}

/**
* Test standalone tests.
*
* @ticket 12009
* @dataProvider data_standalone_inline_script
*/
public function test_standalone_inline_script( $expected, $output, $message ) {
$this->assertSame( $expected, $output, $message );
}

public function data_standalone_inline_script() {
Copy link

Choose a reason for hiding this comment

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

Missing docblock, I know it's a data provider for the test but we should still have docblocks or not?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment about the use of a data provider here.

Copy link
Author

Choose a reason for hiding this comment

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

As mentioned above this will be handled in PR #50.

$data = array();

// If the main script has a `before` inline script; standalone doesn't effect any script.
kt-12 marked this conversation as resolved.
Show resolved Hide resolved
wp_enqueue_script( 'ds-is1-1', 'http://example.org/ds-is1-1.js', array(), null, array( 'strategy' => 'defer' ) );
wp_enqueue_script( 'ds-is1-2', 'http://example.org/ds-is1-2.js', array(), null, array( 'strategy' => 'defer' ) );
wp_enqueue_script( 'ds-is1-3', 'http://example.org/ds-is1-3.js', array(), null, array( 'strategy' => 'defer' ) );
wp_enqueue_script( 'ms-is1-1', 'http://example.org/ms-is1-1.js', array( 'ds-is1-1', 'ds-is1-2', 'ds-is1-3' ), null, array( 'strategy' => 'defer' ) );
wp_add_inline_script( 'ms-is1-1', 'console.log("before one");', 'before', true );
$output = get_echo( 'wp_print_scripts' );
$expected = "<script type='text/javascript' src='http://example.org/ds-is1-1.js' id='ds-is1-1-js' defer></script>\n";
$expected .= "<script type='text/javascript' src='http://example.org/ds-is1-2.js' id='ds-is1-2-js' defer></script>\n";
$expected .= "<script type='text/javascript' src='http://example.org/ds-is1-3.js' id='ds-is1-3-js' defer></script>\n";
$expected .= "<script type='text/javascript' id='ms-is1-1-js-before'>\n";
$expected .= "console.log(\"before one\");\n";
$expected .= "</script>\n";
$expected .= "<script type='text/javascript' src='http://example.org/ms-is1-1.js' id='ms-is1-1-js' defer></script>\n";
array_push( $data, array( $expected, $output, 'All dependency in the chain should be blocking' ) );

// One of the dependency in the chain has a `before` inline script; standalone doesn't effect any script.
kt-12 marked this conversation as resolved.
Show resolved Hide resolved
wp_enqueue_script( 'ds-is2-1', 'http://example.org/ds-is2-1.js', array(), null, array( 'strategy' => 'defer' ) );
wp_enqueue_script( 'ds-is2-2', 'http://example.org/ds-is2-2.js', array( 'ds-is2-1' ), null, array( 'strategy' => 'defer' ) );
wp_enqueue_script( 'ds-is2-3', 'http://example.org/ds-is2-3.js', array( 'ds-is2-2' ), null, array( 'strategy' => 'defer' ) );
wp_enqueue_script( 'ms-is2-1', 'http://example.org/ms-is2-1.js', array( 'ds-is2-3' ), null, array( 'strategy' => 'defer' ) );
wp_add_inline_script( 'ds-is2-2', 'console.log("before one");', 'before', true );
$output = get_echo( 'wp_print_scripts' );
$expected = "<script type='text/javascript' src='http://example.org/ds-is2-1.js' id='ds-is2-1-js' defer></script>\n";
$expected .= "<script type='text/javascript' id='ds-is2-2-js-before'>\n";
$expected .= "console.log(\"before one\");\n";
$expected .= "</script>\n";
$expected .= "<script type='text/javascript' src='http://example.org/ds-is2-2.js' id='ds-is2-2-js' defer></script>\n";
$expected .= "<script type='text/javascript' src='http://example.org/ds-is2-3.js' id='ds-is2-3-js' defer></script>\n";
$expected .= "<script type='text/javascript' src='http://example.org/ms-is2-1.js' id='ms-is2-1-js' defer></script>\n";
array_push( $data, array( $expected, $output, 'Scripts in the chain before the script having before must be blocking.' ) );

return $data;
}

/**
* Test valid async loading strategy case.
*
Expand Down