Skip to content
This repository has been archived by the owner on Dec 1, 2019. It is now read-only.

FIX #29: Add async and defer, async main script #30

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions functions.php
Expand Up @@ -118,6 +118,9 @@ function twentytwenty_theme_support() {
// Custom comment walker
require get_template_directory() . '/parts/classes/class-comment-walker.php';

// Handle JavaScript loading
require get_template_directory() . '/parts/classes/class-script-loader.php';

/**
* Register and Enqueue Styles
*/
Expand Down Expand Up @@ -159,6 +162,7 @@ function twentytwenty_register_scripts() {
$js_dependencies = array( 'jquery' );

wp_enqueue_script( 'twentytwenty-construct', get_template_directory_uri() . '/assets/js/construct.js', $js_dependencies, $theme_version );
wp_script_add_data( 'twentytwenty-construct', 'async', true );

}
add_action( 'wp_enqueue_scripts', 'twentytwenty_register_scripts' );
Expand Down
39 changes: 39 additions & 0 deletions parts/classes/class-script-loader.php
@@ -0,0 +1,39 @@
<?php

/* ---------------------------------------------------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

Can we use docblock style comments instead these big huge blocks?

We should use standard PHP practices.

Copy link
Author

Choose a reason for hiding this comment

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

Afaik this format is the expected format for WordPress coding standards. It's the same format used in previous default themes.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Afaik this format is the expected format for WordPress coding standards. It's the same format used in previous default themes.

Yeah, previous themes had some issues that passed through because TRT wasn't involved 😄We're trying to polish this one out so that we can say: look at 2020 🙂

JAVASCRIPT LOADER CLASS
Allow `async` and `defer` while enqueueing JavaScript. Based on a solution in WP Rig.
--------------------------------------------------------------------------------------------- */

class TwentyTwenty_Script_Loader {

/**
* Adds async/defer attributes to enqueued / registered scripts.
*
* If #12009 lands in WordPress, this function can no-op since it would be handled in core.
*
* @link https://core.trac.wordpress.org/ticket/12009
*
* @param string $tag The script tag.
* @param string $handle The script handle.
* @return string Script HTML string.
*/
public function filter_script_loader_tag( string $tag, string $handle ) : string {
Copy link
Member

Choose a reason for hiding this comment

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

Type hinting against string argument and return value will only work on PHP7.0, and the minimum requirement (at this point) of PHP in WordPress is still 5.6.

https://mlocati.github.io/articles/php-type-hinting.html

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I agree, this theme needs to support PHP 5.6

Type hinting will have to wait for the PHP 7.0 bump

foreach ( [ 'async', 'defer' ] as $attr ) {
if ( ! wp_scripts()->get_data( $handle, $attr ) ) {
continue;
}
// Prevent adding attribute when already added in #12009.
Copy link
Member

Choose a reason for hiding this comment

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

Please update the link on a line of its own to point to the actual Trac ticket

@link https://core.trac.wordpress.org/ticket/12009

if ( ! preg_match( ":\s$attr(=|>|\s):", $tag ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Would stripos to match if an attribute exists in the tag work like regex match? Regex matching is usually a bit slower than matching against a string.

$tag = preg_replace( ':(?=></script>):', " $attr", $tag, 1 );
}
// Only allow async or defer, not both.
break;
}
return $tag;
}

}

$loader = new TwentyTwenty_Script_Loader;
Copy link
Member

Choose a reason for hiding this comment

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

Since this theme is not build using proper OOP principles (encapsulation, polymorphism etc.), I see no reason to put this code in a separate class. There is no dependency injection used anywhere so we can just write procedural code.

Copy link
Author

Choose a reason for hiding this comment

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

Keeping it as a class makes it nicely isolated and easy to remove if/when core finally allows for async and defer. functions.php is already large in this theme. If functionality got split up more, I'd agree to move it, but as of right now it seems more rational to keep it as a stand-alone class.

Copy link
Member

Choose a reason for hiding this comment

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

I understand the rationale, but it just mixes class-based with procedural code, which is a bit inconsistent. I'm all for keeping certain functionalities in separate files and including them in the functions.php for easier management, but I see no benefit in having a class with just one public method in it.

Copy link
Member

Choose a reason for hiding this comment

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

I am actually of the oppinion that encapsulation, even just for the sake of it, is fine. I do not like including a class file that has side effects though.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @dingo-d here that, given how the rest the theme is coded, we shouldn't break that paradigm by introducing a class for functionality like this, as that can make the code harded to understand than everything following one approach.

I suggest introducing a single function attached to the filter.

add_filter( 'script_loader_tag', [ $loader, 'filter_script_loader_tag' ], 10, 2 );