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.

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

Adding support for defer/async from WP 6.3 #2051

Closed
wants to merge 22 commits into from

Conversation

stratease
Copy link
Contributor

@stratease stratease commented Jan 30, 2024

TECENG-48

Utilize defer API available in WordPress 6.3.

See #1991

Required by: the-events-calendar/the-events-calendar#4470

@stratease stratease self-assigned this Jan 30, 2024
…tribe-common into feat/TECENG-48-defer-script
@stratease stratease added the needs tests Needs tests before merging. label Jan 30, 2024
src/Tribe/Assets_Pipeline.php Outdated Show resolved Hide resolved
src/Tribe/Assets_Pipeline.php Outdated Show resolved Hide resolved
src/Tribe/Main.php Outdated Show resolved Hide resolved
src/Tribe/Main.php Outdated Show resolved Hide resolved
src/Tribe/Main.php Outdated Show resolved Hide resolved
@stratease stratease changed the title Feat/teceng 48 defer script Adding support for defer/async from WP 6.3 Jan 30, 2024
tests/wpunit/Tribe/AssetsTest.php Show resolved Hide resolved
tests/wpunit/Tribe/AssetsTest.php Show resolved Hide resolved
tests/wpunit/Tribe/AssetsTest.php Show resolved Hide resolved
tests/wpunit/Tribe/AssetsTest.php Show resolved Hide resolved
tests/wpunit/Tribe/AssetsTest.php Show resolved Hide resolved
tests/wpunit/Tribe/AssetsTest.php Show resolved Hide resolved
tests/wpunit/Tribe/AssetsTest.php Show resolved Hide resolved
tests/wpunit/Tribe/AssetsTest.php Show resolved Hide resolved
tests/wpunit/Tribe/AssetsTest.php Show resolved Hide resolved
tests/wpunit/Tribe/AssetsTest.php Show resolved Hide resolved
@stratease stratease added code review Status: requires a code review. and removed needs tests Needs tests before merging. labels Jan 31, 2024
src/Tribe/Assets.php Outdated Show resolved Hide resolved
@@ -225,7 +225,7 @@ public function load_assets() {
$this,
[
[ 'tribe-accessibility-css', 'accessibility.css' ],
[ 'tribe-query-string', 'utils/query-string.js' ],
[ 'tribe-query-string', 'utils/query-string.js', null, null, [ 'in_footer' => [ 'strategy' => 'defer' ] ] ],
Copy link
Member

Choose a reason for hiding this comment

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

Why are we changing this to defer? Is that fixing a particular bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it was just to optimize the page load. @adamsilverstein ?

Comment on lines +38 to +40
$tag = "<script src='{$dir}/underscore-before.js' defer></script>\n"
. $tag
. "<script src='{$dir}/underscore-after.js' defer></script>\n";
Copy link
Member

Choose a reason for hiding this comment

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

Why defer to these?

@bordoni bordoni added enhance Code could use some enhancements before merging. question Needs an answer to one or more questions before merging. labels Jan 31, 2024
Copy link
Member

@bordoni bordoni left a comment

Choose a reason for hiding this comment

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

I would rather we dont change the stored value for in_footer but how we pass the existing values to the wp_register_script.

@@ -705,7 +710,7 @@ public function register( $origin, $slug, $file, $deps = [], $action = null, $ar

// Clean these
$asset->priority = absint( $asset->priority );
$asset->in_footer = (bool) $asset->in_footer;
$asset->in_footer = is_array( $asset->in_footer ) ? $asset->in_footer : (bool) $asset->in_footer; // Since WordPress 6.3, this parameter accepts an array argument.
Copy link
Member

Choose a reason for hiding this comment

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

Lets not change how in_footer works, when we see the new version of WordPress just modify how we use the values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bordoni we have to change something here or the array will be cast to a bool everytime. Remove the casting altogether? Not sure whether this is here for a particular situation and if it will be safe to remove the casting.

Copy link
Member

Choose a reason for hiding this comment

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

#2054
Don't use in_footer - use existing async/defer params

Copy link
Member

Choose a reason for hiding this comment

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

What I meant was, dont use the in_footer from Tribe Assets, use the already existing async/defer we have plus the in_footer value to compose the array for WordPress.

Copy link
Member

Choose a reason for hiding this comment

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

@bordoni That's what #2054 does

Camwyn added a commit that referenced this pull request Feb 9, 2024
@stratease stratease changed the base branch from release/B24.bittern to master February 12, 2024 06:45
…/tribe-common into feat/TECENG-48-defer-script
@stratease stratease changed the base branch from master to release/B24.cardinal March 4, 2024 23:44
Base automatically changed from release/B24.cardinal to master March 20, 2024 17:12
@bordoni
Copy link
Member

bordoni commented May 18, 2024

Have to close this PR since we will be using https://github.com/stellarwp/assets/ to handle assets, this change will need to be ported over there.

@bordoni bordoni closed this May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code review Status: requires a code review. enhance Code could use some enhancements before merging. question Needs an answer to one or more questions before merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants