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 Async/Defer to the JSConcat script #33

Open
ellm opened this issue May 24, 2017 · 9 comments
Open

Adding Async/Defer to the JSConcat script #33

ellm opened this issue May 24, 2017 · 9 comments

Comments

@ellm
Copy link

ellm commented May 24, 2017

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
Copy link
Member

mjangda commented May 25, 2017

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

@ellm
Copy link
Author

ellm commented May 25, 2017

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
Copy link

mboynes commented May 25, 2017

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
Copy link
Member

mjangda commented Jul 27, 2017

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
Copy link

mehigh commented Dec 20, 2017

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
Copy link
Contributor

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
Copy link

mehigh commented Dec 20, 2017

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
Copy link

mehigh commented Jan 2, 2018

@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.

rinatkhaziev added a commit to rinatkhaziev/nginx-http-concat that referenced this issue May 3, 2018
…ag attributes, the filter gets passed an empty array and has to return an associative or regular array with attributes as keys, additional arguments for the filter are: $href - current batch url, $js_array - additional data for current batch of scripts.

This filter runs on each batch of files; extra arguments are needed to better control attributes being assigned to a resulting tag.

For example, we want to set async/defer for either a specific group of files being concatenated or maybe we want to do a blanket change for every resulting script tag.

Fixes Automattic#33, supersedes Automattic#36
@BrookeDot
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants