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

Improve blocking scripts #6372

Open
Toflar opened this issue Oct 2, 2017 · 35 comments · May be fixed by #1489
Open

Improve blocking scripts #6372

Toflar opened this issue Oct 2, 2017 · 35 comments · May be fixed by #1489
Assignees
Labels

Comments

@Toflar
Copy link
Member

Toflar commented Oct 2, 2017

I guess this is the one problem users struggle with most when analyzing page speed. Page speed insights wants us to put blocking scripts out of the way and I guess it's right about that so we should do something about it. The problem is that almost all of our scripts are loaded in <head>. This is especially complicated for dynamically loaded scripts (when you include a gallery, an accordion etc.) so it's very hard to fix for non-devs and I think it should work better out-of-the-box.

I named this issue RFC as I request for comments here. Please do not just comment a "+1" if you think that needs to be done (you can do that by leaving feedback to the issue itself).
Please comment on concrete ideas how we could solve this.

My idea is to have a simple loader script we include in the <head> but inline (maybe also in <body> at the beginning? To be discussed). Here's a first example (obviously we would minify this):

<head>
    <!-- meta, css and stuff -->
    <script>
    function contaoLoadAsync(url, key) {
        var key = key || 'none';
        var script = document.createElement('script');
        var scripts = d.getElementsByTagName('script')[0];
        script.src = url;
        script.addEventListener('load', function (e) {
           var event = new CustomEvent('contao-script-loaded', { detail: {url: url, key: key} });
           document.dispatchEvent(event);
        }, false);
        scripts.parentNode.insertBefore(script, scripts);
    }
    </script>
</head>

For all the stuff added using $GLOBALS['TL_JAVASCRIPT'] we then output

<script>
contaoLoadAsync('url/to/script.js');
contaoLoadAsync('url/to/gallery.js', 'superduper-gallery-key');
</script>

Our initialize scripts that mostly listen to the domready jquery/mootools events can then be adjusted like so:

<script>
document.addEventListener('contao-script-loaded', function(e) {
    // You can check for your custom key here or check for "url" too
    if (e.detail.key !== 'superduper-gallery-key') {
        return;
    }

    // do initialization here
});

PS: My code is completely untested and just illustrates the general idea. Instead of new CustomEvent() we should use document.createEvent('CustomEvent'); for IE11 compatibility and make sure the async script is as small as possible when minified using more variables etc.

@leofeyer
Copy link
Member

leofeyer commented Oct 2, 2017

Most blocking scripts are CSS files and not JS files according to my experience.

@ausi
Copy link
Member

ausi commented Oct 2, 2017

👍 I agree that the synchronous (blocking) loading of JS in the <head> is a big performance issue and that it should be solved out-of-the-box.

But I see several issues with it:

  1. Events that get registered too late, don’t get executed: If you call addEventListener('contao-script-loaded') after the script was loaded (which could easily happen for cached scripts) the event listener will never get executed.
  2. Execution order: The execution order of dynamically added scripts is undefined, so it could happen that a script that requires jQuery gets executed before jQuery was loaded.

The first issue could be solved by using a custom function instead of an event I think. But the second one will be hard to solve... We probably don’t want to use a module loader and require every script to be a module.

@Toflar
Copy link
Member Author

Toflar commented Oct 3, 2017

Most blocking scripts are CSS files and not JS files according to my experience.

Yeah CSS is blocking too, especially the stuff that is considered to be not critical. But the difference here is that I feel like webdesigners can handle CSS and they know where it is blocking and where it's not. They can also split files if needed, reorganize etc.
For JS, however, it needs a bit more knowledge and for many files you cannot really influence the way they're loaded. It's also not true that JS is not an issue. For the homepage of the official demo we already have 5 blocking JS scripts.

But I see several issues with it:

Of course, there are more issues with it. For example we would need multiple events to be fired when we combine multiple scripts into one 😄 That's why this is an RFC issue. I would like to find a solution and I'm sure there is one. If Contao cannot fix it out-of-the-box, to me that means 99% of all users won't be able to fix it either. That's why I think this is a really relevant topic.

Coming back to your remarks:

1 & 2 sort of go together, I agree. Is there maybe a library that does all of that and is reasonably small?

@Toflar
Copy link
Member Author

Toflar commented Oct 3, 2017

Something like https://github.com/muicss/loadjs could maybe help?

@aschempp
Copy link
Member

aschempp commented Oct 3, 2017

I totally don't understand your approach. The scripts are loaded in header because we do have inline code all over the document to render modules. If that would not be the case, we could simply place everything in the footer. That's why I stopped adding javascript to templates, or better said use $GLOBALS['TL_BODY'] for my script tags. But that will only work if everyone does, and that most likely won't happen soon. I don't see how an event would solve that problem?

@Toflar
Copy link
Member Author

Toflar commented Oct 3, 2017

Obviously all inline code needs to be adjusted from using domready events to the new solution. Templates are not covered by the BC promise so there's no issue with that.

@aschempp
Copy link
Member

aschempp commented Oct 3, 2017

So why would we need anything and an event in the header at all? Just add everything to body?

@Toflar
Copy link
Member Author

Toflar commented Oct 3, 2017

Please elaborate on how your solution would work, please.

@aschempp
Copy link
Member

aschempp commented Oct 3, 2017

$GLOBALS['TL_BODY'][] = '<script src="jquery"></script>';

You could also add async and just keep it in the header. But again, it will break like all pages…

@Toflar
Copy link
Member Author

Toflar commented Oct 3, 2017

And how do inline gallery scripts wait until this is loaded?

@aschempp
Copy link
Member

aschempp commented Oct 3, 2017

well they don't. Same as with the event, they must be added after the initial scripts. Basically, do not inline. Use TL_BODY for all scripts.

@aschempp
Copy link
Member

aschempp commented Oct 3, 2017

An alternative would be to introduce a real module loader, not sure how that would work with such a setup though. But we sure should not invent our own loading system…

@Toflar
Copy link
Member Author

Toflar commented Oct 4, 2017

Use TL_BODY for all scripts.

That doesn't work for combined scripts that's why I wanted to move all $GLOBALS['TL_JAVASCRIPT'] registered files into the body and invent an event/callback based system that makes sure dependencies are considered and loaded accordingly. In best case all a developer has to do is adjust their inline scripts from the "domready" or no event to some custom one.

@leofeyer leofeyer assigned leofeyer and Toflar and unassigned leofeyer Oct 4, 2017
@leofeyer leofeyer added feature up for discussion Issues and PRs which will be discussed in our monthly Mumble calls. labels Oct 4, 2017
@aschempp
Copy link
Member

aschempp commented Oct 5, 2017

Why would that not work for combined scripts? Where's the difference between loading in header and footer if the order is correct?

@leofeyer leofeyer removed the up for discussion Issues and PRs which will be discussed in our monthly Mumble calls. label Oct 5, 2017
@michapietsch
Copy link

What about running the whole rendered html through a filter that moves all script tags to the bottom while keeping their order? I have such a solution working in Magento systems: https://github.com/bobbyshaw/magento-footer-js

@Toflar
Copy link
Member Author

Toflar commented Oct 11, 2017

That would certainly work somehow but it's a slow and possibly unreliable solution (especially when you use regular expressions the way magento-footer-js does instead of DOM traversal) and I would like to see if there are other options first :) So that would be my last resort. But maybe still better than nothing at all, we'll see :)

@fritzmg
Copy link
Contributor

fritzmg commented Oct 11, 2017

Doesn't Theme+ do this?

@Toflar
Copy link
Member Author

Toflar commented Oct 11, 2017

There are quite a few extensions that apply stuff on the resulting HTML and I think we don't need this.

@fritzmg
Copy link
Contributor

fritzmg commented Oct 11, 2017

None of them are perfect though imho.

@aschempp
Copy link
Member

aschempp commented Oct 11, 2017 via email

@ausi
Copy link
Member

ausi commented Oct 11, 2017

AFAIK loading the scripts in the head asynchronously is much better than loading them in the footer synchronously. Async should allow more parallelism and doesn’t block the domready event.

I found an article that describes how to load scripts asynchronously and keep the desired execution order: https://www.html5rocks.com/en/tutorials/speed/script-loading/. IMO that would be the best option for our use-case.

@michapietsch
Copy link

Is that article from 2013 still valid?

BTW I liked the part where he writes:

I find this article depressing.
The situation is depressing and you should feel depressed. There’s no non-repetitive yet declarative way to download scripts quickly and asynchronously while controlling the execution order.

;)

Four years later "subresource" is still not supported by all browsers and so scripts may be hidden from preloaders. It's a trade-off.

Well, at the moment I manually extract scripts, process them via task runner and add one resulting script as async. I include basic critical CSS to style e. g. slides before the actual slider is loaded (which is also done async).

At least I can confirm that the above mentioned method to automatically move everything to the bottom is working on production sites and saves me some time as long as it's running smoothly.

@Aybee
Copy link
Contributor

Aybee commented Oct 11, 2017

I stumble upon this

@Toflar The problem is that almost all of our scripts are loaded in head. This is especially complicated for dynamically loaded scripts (when you include a gallery, an accordion etc.) so it's very hard to fix for non-devs and I think it should work better out-of-the-box.

But colorbox, accordion ... are not loaded in the head? Can you explain what you mean?

@leofeyer
Copy link
Member

What's the status of this one?

@aschempp
Copy link
Member

aschempp commented Sep 3, 2018

Maybe we should promote the use of Symfony Encore (and possibly add an option to include assets in the layout) instead of trying to improve on legacy implementations?

@koertho
Copy link

koertho commented Dec 20, 2018

Maybe we should promote the use of Symfony Encore (and possibly add an option to include assets in the layout) instead of trying to improve on legacy implementations?

@aschempp We have an bundle doing exactly this: https://github.com/heimrichhannot/contao-encore-bundle . Maybe it can act as a first approach for having a contao core-supported way?

@aschempp
Copy link
Member

Looks pretty interesting! Have you considered creating a pull request to have something merged to the core?

leofeyer referenced this issue in contao/core-bundle Dec 20, 2019
…123)

Description
-----------

Closes #618 

Complementary commit: contao/tcpdf-bundle@0c377c3

Commits
-------

10c2d6d1 Do not strip forms in the ModuleArticle::generatePdf() method (see #618)
@ausi ausi linked a pull request Mar 4, 2020 that will close this issue
@Metis77
Copy link

Metis77 commented Mar 10, 2020

Why building an own script loader?
HTML script attributes async and defer already offer non blocking script loading.
defer also takes care of the execution order.

Only thing that has to be changed this way, is inline script.
This way inline scripts should contain only data variables but not the functions (like an accordeon initialization) that rely on loaded scripts. those functions should also be loaded deferred / non blocking.

an example

<script>
const myCustomAccordeon = {
    'foo':'bar'
}
</script>
<script src="assets/jquery.js" defer></script>
<script src="assets/contaoCustomAccordeon.js" defer></script>

myCustomAccordeon.js can than just use the inline data.

@ausi
Copy link
Member

ausi commented Mar 10, 2020

defer also takes care of the execution order.

That is correct but defer also delays script execution until the full HTML is downloaded and parsed.

Using async = false as in #1489 keeps execution order while allowing the scripts to execute as soon as possible.

@Metis77
Copy link

Metis77 commented Mar 10, 2020

That is correct but defer also delays script execution until the full HTML is downloaded and parsed.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script#attr-defer
This Boolean attribute ( defer ) is set to indicate to a browser that the script is meant to be executed after the document has been parsed (when the DOM is ready), but before firing DOMContentLoaded.

true, it is delayed a little ... but in a way I usually want.
I want the DOM to be ready as soon as possible. And only after this I want JS to do its thing, like DOM manipulations.
In most cases I do not want JS to be executed just as soon as possible.

I think, sticking to default HTML script defer would keeps things simpler, too.

Of course all this is just my opinion.
The above PR would be a major improvement anyway.

@fritzmg
Copy link
Contributor

fritzmg commented Mar 11, 2020

In most cases I do not want JS to be executed just as soon as possible.

You want that with above the fold content, e.g. click event handlers for menu buttons, and sliders.

@Metis77
Copy link

Metis77 commented Mar 11, 2020

You want that with above the fold content, e.g. click event handlers for menu buttons, and sliders.

Hm I thought you can not register a click handler on a dom element that is not ready, yet.
So would you not have to wait for the dom ready for doing this?

@fritzmg
Copy link
Contributor

fritzmg commented Mar 11, 2020

No, you can initialize it directly after or even within the element.

@Metis77
Copy link

Metis77 commented Mar 11, 2020

No, you can initialize it directly after or even within the element.

Ah, good to know this.

I would still prefer the default HTML defer approach.
While inline JS would not block the loading, it would still block the UI Threat (DOM construction ):
https://developers.google.com/speed/docs/insights/BlockingJS?hl=de#deferJS

But both (above PR and defer) would be much better than the current way.

@leofeyer
Copy link
Member

leofeyer commented Jun 11, 2020

FYI, we have discussed this again at the developer's meeting in February, 2020, and we have agreed that it does not have a high priority at the moment. Also, before we can implement the feature, we have to get rid of the MooTools and jQuery dependencies in the front end.

@leofeyer leofeyer changed the title [RFC] Improve blocking scripts Improve blocking scripts Sep 1, 2023
@leofeyer leofeyer transferred this issue from contao/core-bundle Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants