Fixes #14817 - load JS only in relevant pages

[tbrisker]

The proxy_status.js and about.js files should only be loaded on relevant
pages.

[dLobatog]

I'm not sure, this ought to make it slower. For starters, by the time you're in /smart_proxies or /about , your browser should have fetched application.js. This means it's loaded from cache so it would take 0s to download it.

Even if the browser downloads these two parallel (it does), it opens a new TCP connection and it will have to evaluate two scripts, so TTP (time to paint) is a bit higher. Why do you think this is a problem?

[tbrisker]

It is true that loading another file may be slightly slower, but I think the difference will not be noticeable in a production environment. The reason I proposed this change is that we currently execute a lot of JS on every page load (in fact, almost all of our js gets executed for every contentLoad or documentLoad event), leading to longer execution times for code that does not need to run at all. Another issue with loading and executing everything for all pages is that sometimes code that is written with one page in mind may cause side effects on different pages that aren't expected due to the way our code is currently structured.

[iNecas]

+1 for loading the stuff only when it makes sense. I've seen the issues
with the pages getting unaffected by something else. For sure the proper
solution would be having a namespacing solution involved, but we are not
there yet. Anyway, having more explicit way to see that some js is used on
just some pages is a big + for me.

On Sat, Apr 30, 2016 at 2:46 PM, Tomer Brisker notifications@github.com
wrote:

It is true that loading another file may be slightly slower, but I think
the difference will not be noticeable in a production environment. The
reason I proposed this change is that we currently execute a lot of JS on
every page load (in fact, almost all of our js gets executed for every
contentLoad or documentLoad event), leading to longer execution times for
code that does not need to run at all. Another issue with loading and
executing everything for all pages is that sometimes code that is written
with one page in mind may cause side effects on different pages that aren't
expected due to the way our code is currently structured.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#3466 (comment)

[gailsteiger]

This is an important change. The proxy_status.js file has handlers attached to a general document event and a general window event. Since this file is concatonated into application.js, it is loaded and run on every page load. These event handlers are attached to every page and have side effects, some of which I've encountered.

[tbrisker]

@iNecas @dLobatog any chance we can advance this? I believe the added time to load the extra js is minor compared to the benefit of starting to modularize our js code, with the eventual target of moving everything we can into es2015 modules and cleaning up the global scope.

[iNecas]

@tbrisker agreed 100%. I'm not a big fan of micro-optimization, especially if it goes against design benefits.

[ohadlevy]

@iNecas anything blocking this from being merge then? ;)

[iNecas]

Was waiting for @dLobatog to finish the discussion. Giving it day.

[dLobatog]

@iNecas Thanks, I'll measure & post the results, sorry. I measured this kind of stuff a long time ago for http://projects.theforeman.org/issues/14117 and splitting JavaScript code even more added to the problem. These are not micro-optimizations, 'time to paint' can go down severely (1 second) when not all of the necessary JS code has been loaded.

Give me until the end of today, and if I have not published measurements that support that, feel free to merge tomorrow morning or I can do it myself if it's not that much slower.

[ohadlevy]

@dLobatog i would also like to mention this creates a challenge for #3603 as the event is trigger on every page regardless. so it also solves a problem and not a pure performance issue.

[dLobatog]

Here are some measurements I did with the Chrome timeline:

• Before splitting up JS (current develop)
  • nocache
    • dashboard
      • 460ms to 660ms application.js loads
      • paint in 880ms
    • smart-proxies
      • paint in 880ms
    • smart-proxy
      • 117ms to fetch application.js, ~200ms to 300ms
      • paint in 880ms
  • withcache
    • dashboard
      • same, paint around 880ms
    • smart-proxies index
      • same, paint around 880ms
    • smart-proxy
      • same, paint around 880ms
• After splitting up JS (with this PR)
  • nocache
    • dashboard
      • 1000ms time to paint
    • smart-proxies index
      • 830ms time to paint
    • smart-proxy
      • 660ms time to paint!
  • cache
    • dashboard
      • 857ms time to paint
    • smart-proxies index
      • 628 ms time to paint
    • smart-proxy
      • 665 ms time to paint

I saved a few of the timelines in case you want to inspect them, but I did nothing fancy other than make Chrome save screenshots and measure paint in the Timeline tab of the devtools, also looked a bit at the network tab to see how it was retrieving these files. In any case it doesn't seem to make a major difference.

Merging!

[dLobatog]

Merged as ede1a30, thanks @tbrisker, also @iNecas @gailsteiger @ohadlevy !