Automattic / nginx-http-concat

WordPress plugin to perform CSS and JavaScript concatenation of individual script files into one resource request.
GNU General Public License v2.0
115 stars 32 forks source link

Adding Async/Defer to the JSConcat script #33

Open ellm opened 7 years ago

ellm commented 7 years ago

There does not seem to be a way to filter the resulting script block. If it could be filtered then attributes could be added to the concatenated script block such as performance related async or defer.

mjangda commented 7 years ago

I guess we're missing a matching version of the ngx_http_concat_style_loader_tag filter for javascript.

ellm commented 7 years ago

Thanks @mjangda. That makes sense. Would updating https://github.com/Automattic/nginx-http-concat/blob/master/jsconcat.php#L161 to something like below be acceptable?

$tag = "<script type='text/javascript' src='$href'></script>\n";
echo apply_filters( 'ngx_http_concat_script_loader_tag', $tag, $href );
mboynes commented 7 years ago

The solution might need to be more robust than that. The async and defer attributes should be able to be set on the enqueued asset and then the concat utility would group them. Otherwise, you might end up async or defering a script which, for one reason or another, needs to be loaded synchronously. Groups could be leveraged for this, though they're a bit clunky.

Also relevant: https://core.trac.wordpress.org/ticket/22249

mjangda commented 7 years ago

Yeah, might be good to have this built-in to the plugin using an approach like this: https://gist.github.com/georgestephanis/2a84bc55ad23f4dec2cf2464109add59

That way, all sites have to worry about is called wp_script_add_data.

mehigh commented 6 years ago

We hit this snag ourselves. All of the JS assets that we do not want to be render-blocking because we want a site render fast ends up slowing down the paint on production because the concatenation doesn't cater for defers.

@mjangda - would it be a good investment of my time if I went in and went forward with your suggestion from Jul 27 and come back with a PR? If you already have this in the pipeline, at least let me know when it's expected to come through, and whether there's anything else I can help with to get it moving forward.

joshbetz commented 6 years ago

If you already have this in the pipeline, at least let me know when it's expected to come through

Nothing currently in the pipeline.

mehigh commented 6 years ago

I should take this as having a green light to go ahead and implement this, right... having an implementation suggestion from mjangda which I'm OK to move forward with anyways.

I have some time for some contributions, and would definitely prefer to spend it on items which can speed up websites.

mehigh commented 6 years ago

@mboynes I've refreshed https://core.trac.wordpress.org/ticket/22249 you've referenced above and also tested it. It contains good work, hope it will eventually move forward and get merged in.

I'm going to get accommodated to concat groups now and see how a solution can start to shape up.

BrookeDot commented 1 year ago

WordPress 6.3 added support for defer and async to the wp_register_script function: https://make.wordpress.org/core/2023/07/14/registering-scripts-with-async-and-defer-attributes-in-wordpress-6-3/

If possible we should use the strategy value of the registered script to determine the strategy. It could be used in combination with #36 which adds a filter, we could check for the values and if present add them to the filter dynamically.