FIX #29: Add async and defer, async main script
#30
Conversation
…async main script.
|
This is a neat trick 😄 Think we instantiate the loader somewhere else though? Perhaps inside an |
| @@ -0,0 +1,39 @@ | |||
| <?php | |||
|
|
|||
| /* --------------------------------------------------------------------------------------------- | |||
There was a problem hiding this comment.
Can we use docblock style comments instead these big huge blocks?
We should use standard PHP practices.
There was a problem hiding this comment.
Afaik this format is the expected format for WordPress coding standards. It's the same format used in previous default themes.
There was a problem hiding this comment.
There was a problem hiding this comment.
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 🙂
| * @param string $handle The script handle. | ||
| * @return string Script HTML string. | ||
| */ | ||
| public function filter_script_loader_tag( string $tag, string $handle ) : string { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I agree, this theme needs to support PHP 5.6
Type hinting will have to wait for the PHP 7.0 bump
| continue; | ||
| } | ||
| // Prevent adding attribute when already added in #12009. | ||
| if ( ! preg_match( ":\s$attr(=|>|\s):", $tag ) ) { |
There was a problem hiding this comment.
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.
|
|
||
| } | ||
|
|
||
| $loader = new TwentyTwenty_Script_Loader; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| if ( ! wp_scripts()->get_data( $handle, $attr ) ) { | ||
| continue; | ||
| } | ||
| // Prevent adding attribute when already added in #12009. |
There was a problem hiding this comment.
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
|
|
||
| } | ||
|
|
||
| $loader = new TwentyTwenty_Script_Loader; |
There was a problem hiding this comment.
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.
|
We talked about this and we want to roll this in. I hope it encourages others to follow this newer best practice :) I still vote we keep it as a class if only for the sake of ease of reuse/removed when this lands in core. |
|
Hey @mor10 I grabbed this branch and made a couple adjustments to it. Moved/renamed the class to be beside others, moved the instantiation an filter to be alongside other theme support type items. It would be great if I could get a little feedback from you on it. The branch I've got that in is here: https://github.com/pattonwebz/twentytwenty/tree/issue/29-2/async-defer-javascript - it's the 5 most recent commits in this branch that are relevant |
|
Closing because this was merged in #339 |
asyncanddeferas options to control JS loading viawp_script_add_data./assets/js/construct.jstoasyncto improve performance and reduce render blocking.