- Issue created by @xamount
- 🇹🇹Trinidad and Tobago xamount
Coming to thinking of it, proposed resolution #1 is not really optimal.
Example of a js defined library:
mycustomjslibrary: header: true js: dist/js/brands.min.js: { attributes: { defer: true }}If you server pushed this js, then the defer option is kinda irrelevant.
- 🇮🇳India abhinand gokhala k
I have created a configuration for exclude js push.
/admin/config/http2_server_push - 🇹🇹Trinidad and Tobago xamount
Thanks Abhinand,
I have done an initial review.
For users that already have this module enabled, if we were to introduce this patch, they will get this error upon drush cr:
$ drush cr [warning] Trying to access array offset on value of type bool Http2ServerPushServiceProvider.php:34So I believe you need to take care of the case where this setting has not been set initially (provide a default value of FALSE ?).
After I enabled the setting "exclude js", I can see that the JS is excluded. But when I disabled the setting, the JS is still excluded. I believe you need to revise the if() logic.
- 🇹🇹Trinidad and Tobago xamount
Also, it will be better to set the path correctly to add it to the Development menu at admin/config/development. Right now, users will need to manually type in the config settings form path
- 🇮🇳India abhinand gokhala k
Created menu link under admin/config/development.
And solved the warning error.And attached the testing video.
- last update
over 1 year ago Patch Failed to Apply - last update
over 1 year ago Build Successful - Status changed to Needs review
over 1 year ago 8:47pm 21 April 2023 - last update
over 1 year ago Build Successful - Status changed to RTBC
over 1 year ago 2:45am 23 April 2023 - 🇹🇹Trinidad and Tobago xamount
Thank you very much @Abhinand Gokhala K
Great work! I tested the latest patch and all the issues have been resolved and it works as expected now.
- last update
over 1 year ago 1 pass - 🇫🇷France prudloff Lille
Instead of not pushing JS at all, wouldn't it make sense to only avoid pushing deferred JS?
The attached patch does that.JS is not render blocking
JS in the head without an async or defer attribute is render blocking (at least according to Lighthouse).
- 🇹🇹Trinidad and Tobago xamount
Hi @prudloff
Yes you are absolutely right: "JS in the head without an async or defer attribute is render blocking".
However, I tested your patch and here is what happens.
<html> <head>....</head> <body> <script src="somecode.js">...</script>. //this non-critical JS gets pushed even though it's not in the <head>. </body> </html> - Status changed to Needs review
over 1 year ago 5:11pm 22 May 2023 - last update
over 1 year ago Build Successful - 🇫🇷France prudloff Lille
My patch was only looking for the defer attribute.
I updated it to only push JS in the head without defer or async attribute. - Status changed to Needs work
over 1 year ago 1:05am 23 May 2023 - 🇹🇹Trinidad and Tobago xamount
Thank you @prudloff
Almost works now.
You just need to update the last return statement to:
return $this->jsCollectionRenderer->render($js_assets);Otherwise the other JS on the page will be excluded from being rendered.
- 🇹🇹Trinidad and Tobago xamount
Or at least create a variable for it so we don't need to call $this->jsCollectionRenderer->render($js_assets) twice.
- Status changed to Needs review
over 1 year ago 8:24am 23 May 2023 - last update
over 1 year ago Build Successful - last update
over 1 year ago 1 fail - 🇹🇹Trinidad and Tobago xamount
Just fixing a php code sniffer error.
Attached is the updated patch.
The last submitted patch, 17: http2_server_push-3350385-17.patch, failed testing. View results →
- Status changed to Needs work
about 1 year ago 2:42pm 20 October 2023 - Status changed to Needs review
about 1 year ago 2:45am 31 October 2023 - last update
about 1 year ago 1 fail The last submitted patch, 19: http2_server_push-3350385-18.patch, failed testing. View results →