SMF icon indicating copy to clipboard operation
SMF copied to clipboard

Should defer loading of as much JavaScript as possible

Open jdarwood007 opened this issue 3 years ago • 8 comments

This is a discussion, not a bug report. But it would introduce some changes to the code.

I tested issuing a defer on all javascript files except the jquery file. When I did this, SMF has errors on pages because the scripts may not be available yet when they are encounter during page loading. This simply means, it is impossible currently for us to issue defer or async on javascript files to improve page loading speeds.

There is two solutions to this.

  1. We don't defer any javascript files ever, causing performance penalties as those files must finish downloading before continuing to render a page.
  2. We wrap all inline javascript in a check such as
  • $(document).ready({function() {....});
  • window.addEventListener("DOMContentLoaded", function() {....});

I think going with the second choice and using the standard javascript loader method is better because it works and will even function without jquery being loaded. Which if we wrap jquery objects in the second method as well, jquery can even be deferred. What this means is we do not block downloading or processing of a page while the javascript file is downloading in the background and continue on with everything. With the even listener, once the page has almost finished loading, we can execute the javascript to bring in those pieces that enhance the overall usage of SMF.

I started to look around places and notice we do most of our inline javascript with just standard script tags on the pages. Instead of calling the built in function addInlineJavaScript(string $code, bool $defer) Ideally we would move as much as we can to the inline javascript with a defer, which sends all this to the end of the page and defers it automatically with the appropriate event listener.

As it is late in RC stages, this should have a discussion before we even build a PR. I feel it is worth getting this fixed because it allows defer on the javascript to occur, improving overall performance on a site.

jdarwood007 avatar Feb 26 '21 14:02 jdarwood007

This reminds me of the Cloudflare issue we had on the support forum: https://www.simplemachines.org/community/index.php?topic=576192.msg4076986#msg4076986

Good idea. Sounds like substantive changes, though, because yes, it has always been done every which way in smf.

I would NOT want to put an RC4 or Final label on something that potentially big & disruptive.

sbulen avatar Feb 26 '21 14:02 sbulen

Not only inline scripts everywhere, but also inline events. Also, no jQuery please, kthx.

I'm definitely for it, probs just a question of when.

live627 avatar Feb 26 '21 14:02 live627

I think inline events should be safe as they won't be executed until the event triggers them, such as onclick="". But because of the change from a global scope to a local scope in the anon function, we would need to update most of those inline events to use window.

jdarwood007 avatar Feb 26 '21 19:02 jdarwood007

Inline scripts are everywhere... Every template is full of them. They are even in some source files.

This feels more like a set of standards to follow for 3.0...

sbulen avatar Mar 15 '21 18:03 sbulen

I don't understand what the issue is here. We already have full support for deferred, asynchronous, and standard loading of JavaScript files, and for deferred and standard loading of inline JavaScript (see #4640, #4656, and #5606).

Deferred and asynchronous files are given the defer or async attributes as appropriate: https://github.com/SimpleMachines/SMF2.1/blob/751a19d7a7caa560219467b5b322ce4b39e5275e/Sources/Subs.php#L4357-L4370

We even maintain defer and async for minified files: https://github.com/SimpleMachines/SMF2.1/blob/751a19d7a7caa560219467b5b322ce4b39e5275e/Sources/Subs.php#L4374-L4387

The deferred inline JavaScript is wrapped in window.addEventListener("DOMContentLoaded", function() {....});, as shown here: https://github.com/SimpleMachines/SMF2.1/blob/751a19d7a7caa560219467b5b322ce4b39e5275e/Sources/Subs.php#L4392-L4404

In light of all that, is the issue here just that we ought to convert any remaining hardcoded inline JavaScript in the templates into addInlineJavaScript() calls?

Sesquipedalian avatar Jul 19 '21 18:07 Sesquipedalian

@Sesquipedalian I forced all files to issue a defer as a test, which means that in the browser context, they won't execute the javascript until the page has been loaded (i.e. in simple terms "

" was received). But because of this, it seems to cause a lot of problems with SMF.

The thoughts are correct though. This should go int a future version of SMF and we should take time then to correctly force all javascript to defer and properly load (with by using ready events). We can then default all of our js to use defers and thus improve page loads.

jdarwood007 avatar Jul 20 '21 21:07 jdarwood007

Now I think I understand. It seems like the previously unstated presupposition here is that we should make it a goal to defer loading all our JavaScript. That is a good goal, and everything else I'm reading here makes a lot more sense in that context.

(Without that context, this issue seemed to be saying, "If all the JavaScript is blindly forced to be deferred, it breaks," to which my response was, "Well, yeah. It isn't all meant to be deferred. Don't do that.")

If I am understanding correctly now, then this issue is essentially a feature request rather than a bug report. Nothing is broken now, but for the sake of better performance, it would be great if we could rewrite our JavaScript loading so that all of it was deferred. Is that right? If so, I agree that it is a good goal for the future, although I am not sure that absolutely everything can or should be deferred.

Sesquipedalian avatar Jul 20 '21 22:07 Sesquipedalian

Yes it would be a feature request. I reported it because we should be doing defer all javascript. It is a recommended standard to increase page loading times since the browser can finish rendering the DOM while the javascript is downloaded an then executed. Without defer the file must download first before finishing the DOM load. Although not many sites implant this. Because of the amount of changes, its best to break it in a future version and work our way through all broken javascript features.

jdarwood007 avatar Jul 20 '21 23:07 jdarwood007