[STALLED] Feature/4172 eliminate render blocking resources #4604

rfultz · 2021-05-06T19:18:52Z

Summary

Resolves Optimization: Eliminate render-blocking resources #4172
deferring script loads to improve our page performance
preloading the homepage hero image

Required reviewers

Two front-end and/or UX?

Impacted areas of the application

Preloading the homepage hero has no downside.

Now we're delaying script loads for a few templates. The most likely complication is that one of the scripts won't have loaded when it was expected, meaning a component isn't activated or isn't ready to when a user thinks it should have been. This could happen the first time a site visitor requests a script after our last deploy (pages that share scripts will use their cached versions) and when there's a slow connection.

The affected templates are for the homepage, elections, election search, and landing.

The pages

Screenshots

No visual changes

Related PRs

None

How to test

pull the branch
./manage.py runserver
Make sure the interactive parts work: (menus, search, widgets, glossary, etc)
- homepage
- election-lookup (homepage and /data/elections/)
- elections
- landing
- Slow the connection speed and try again
In Inspector, under the Network tab, change "No throttling" to something else and re-check the pages. They'll load more slowly and may not become useable until the last file is loaded and processed. But they should become interactive just like usual (just a little later than we're expecting).

For the homepage, Lighthouse should no longer mention "eliminate render blocking resources"

patphongs

@rfultz Looks like lighthouse is still flagging the elections.css in the "Eliminate render blocking" report. Is there a way to defer that?

<link rel="stylesheet" type="text/css" href="{% asset_for_css 'elections.css' %}">

patphongs · 2021-05-17T19:29:25Z

@rfultz Looks like lighthouse is still flagging the elections.css in the "Eliminate render blocking" report. Is there a way to defer that?

<link rel="stylesheet" type="text/css" href="{% asset_for_css 'elections.css' %}">

Per @rfultz, this might be addressed in the clean up work in this PR.

patphongs

Looks good!

johnnyporkchops

The cleanURI() function that we use on the homepage to keep the querystring out of the URL does not seem to work if we defer election-lookup.js

It needs jQuery. Specifically, the .on method. I tried to start writing a vanilla JS version, but it gets tricky trying to emulate $.on. Should we replace it with vanilla JS or is there a better way around deferring the needed asset?

FYI- I had to open a new icognito window each time to definitively test removing/replacing defer on the script tag

Also weirdly, election-lookup js was not flagged for Eliminate render-blocking resources by lighthouse when removing defer (?)

johnnyporkchops · 2021-05-18T00:34:42Z

fec/home/templates/home/home_page.html

-<script src="{% asset_for_js 'election-lookup.js' %}"></script>
+<script defer src="{% asset_for_js 'home.js' %}"></script>
+<script defer src="{% asset_for_js 'bythenumbers.js' %}"></script>
+<!-- <script defer src="{% asset_for_js 'dataviz-common.js' %}"></script> -->


Do you want to leave this commented tag in here?

Oof. Nope! Good catch!

johnnyporkchops · 2021-05-18T00:53:53Z

fec/home/templates/home/home_page.html

+<script defer src="{% asset_for_js 'home.js' %}"></script>
+<script defer src="{% asset_for_js 'bythenumbers.js' %}"></script>
+<!-- <script defer src="{% asset_for_js 'dataviz-common.js' %}"></script> -->
+<script defer src="{% asset_for_js 'election-lookup.js' %}"></script>


The cleanURI() function that we use on the homepage to keep the querystring out of the URL does not work if we defer election-lookup.js

It needs jQuery. Specifically, the .on method. I tried to start writing a vanilla JS version, but it gets tricky trying to emulate $.on. Should we replace it with vanilla JS or is there a better way around this?

What if we change election-lookup to async?

I changed this to native on another ticket. Should I merge that one into this one?

I did try async and got same problem

I changed this to native on another ticket. Should I merge that one into this one?

Whatever fixes it is fine with me

johnnyporkchops · 2021-05-18T17:22:19Z

Thanks @rfultz .
Now it is removing the querystring on-load, but then if you interact with map, it keeps only state or only the district.

If we ended up deciding that its OK to have querystrings once someone interacts with the map, we would probably want to have it register every parameter they choose

Update: I just realized that this ^^^ might still be a problem because the bar charts below the map also add querystrings to the URL which is confusing--part of the reason we scrub the URL in the first place.

johnnyporkchops · 2021-05-19T05:09:34Z

@rfultz this works if we put it back on home_page.html inside a window.onload function.

<script>
window.onload = function () {
  $(function() {
    cleanURI()
      function cleanURI(){
        const uri = window.location.toString();
        if (uri.indexOf('?') > 0) {
          window.history.replaceState({}, document.title, location.href.split('?')[0]);
        }
      }
      $('#main').on('change ', 'input, select', function(){
        cleanURI();
      })
      //handle Chrome insconsistency with History API
      $('.js-office').val('S');
      $('.js-election-year').val('2022')
      $('.js-chart-toggle').filter('[value=receipts]').prop('checked', true)
  })
}
</script>

Works like this when in home_.page.html
If you were to move it to home.js, it loses the race condition intermittently, which I have not seen happening when putting it directly in home_page.html. Let me know what you find if you test it.
On some browsers I could remove jQuery's shorthand $(function() wrapper( which is the same as $(document).ready(function() ) and just wrap it in window.onloadbut then Opera does not work. So even though it might feel redundant to have two onloads, I think that is the most ubiquitious coverage.

Let me know what you think

codecov-commenter · 2021-05-20T20:10:59Z

Codecov Report

Merging #4604 (46baead) into develop (c4493b1) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #4604      +/-   ##
===========================================
- Coverage    75.84%   75.83%   -0.02%     
===========================================
  Files          125      125              
  Lines         7660     7660              
  Branches       618      618              
===========================================
- Hits          5810     5809       -1     
- Misses        1850     1851       +1

Impacted Files	Coverage Δ
fec/fec/static/js/modules/calendar.js	`92.02% <0.00%> (-0.73%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4493b1...46baead. Read the comment docs.

…n home page

rfultz · 2021-06-01T13:45:19Z

fec/home/templates/home/home_page.html

-    );
-  }
+window.onload = function () {
+  $(function() {


So we'd be using the the native onload event to trigger a jQuery function ($(function)) that creates another function (cleanURI), calls it, and calls four other jQuery commands (on, val, val, and filter.prop)?

Yes. Not ideal but since it was the only working solution yet, it's worth logging it to the conversation should we need to fall back on it. And only requires adding two lines of code to the existing. Do the latest changes committed since work? Were you able to confirm that before pushing.?

patphongs · 2021-06-07T20:50:00Z

We're getting some errors here, so we can't merge this yet:

Webpack error attributed to the defer on the vendor.js.

calc-admin-fines-modal-02db7d5fde06fe303948.js:1 Uncaught ReferenceError: webpackJsonp is not defined
    at calc-admin-fines-modal-02db7d5fde06fe303948.js:1

Updating the district dropdown does not strip the URL. All the other dropdowns do strip the URL as expected.

patphongs

Some things need to be addressed first: #4604 (comment)

rfultz added 2 commits May 6, 2021 14:38

Preload the homepage hero image

3d275b1

Defer JavaScript files

a8475f5

rfultz added Please review Work: Front-end labels May 6, 2021

rfultz requested review from johnnyporkchops, patphongs and JonellaCulmer May 6, 2021 19:18

rfultz self-assigned this May 6, 2021

Defer other homepage/global scripts

d7bfbb0

patphongs reviewed May 17, 2021

View reviewed changes

patphongs approved these changes May 17, 2021

View reviewed changes

patphongs added the Before release label May 17, 2021

johnnyporkchops requested changes May 18, 2021

View reviewed changes

rfultz added 3 commits May 18, 2021 11:42

Remove commented script tag

8fb6ebc

cleanURI from homepage template jQuery to ES6 home.js

5c9c828

Add timer to remove query params for homepage

0e18cd5

rfultz added 2 commits May 20, 2021 10:53

Remove timer, remove params after everything is loaded

6fcdf9f

Move the listeners into home.js

46baead

Move scrubURI from home.js to home_page.html

d992b00

JonellaCulmer removed their request for review May 28, 2021 14:10

add win.onload to make sure jquery cleanURI script works with defer o…

1eefc42

…n home page

rfultz commented Jun 1, 2021

View reviewed changes

rfultz added 2 commits June 1, 2021 16:16

Fix errors when election-map is on homepage

9ef5020

Another attempt at a native solution

594c179

patphongs requested changes Jun 7, 2021

View reviewed changes

patphongs removed the Before release label Jun 7, 2021

rfultz changed the title ~~Feature/4172 eliminate render blocking resources~~ [STALLED] Feature/4172 eliminate render blocking resources Jul 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[STALLED] Feature/4172 eliminate render blocking resources #4604

[STALLED] Feature/4172 eliminate render blocking resources #4604

rfultz commented May 6, 2021

patphongs left a comment

patphongs commented May 17, 2021

patphongs left a comment

johnnyporkchops left a comment •

edited

Loading

johnnyporkchops May 18, 2021

rfultz May 18, 2021

johnnyporkchops May 18, 2021

rfultz May 18, 2021

rfultz May 18, 2021

johnnyporkchops May 18, 2021

johnnyporkchops May 18, 2021 •

edited

Loading

johnnyporkchops commented May 18, 2021 •

edited

Loading

johnnyporkchops commented May 19, 2021 •

edited

Loading

codecov-commenter commented May 20, 2021

rfultz Jun 1, 2021

johnnyporkchops Jun 2, 2021

patphongs commented Jun 7, 2021

patphongs left a comment

[STALLED] Feature/4172 eliminate render blocking resources #4604

Are you sure you want to change the base?

[STALLED] Feature/4172 eliminate render blocking resources #4604

Conversation

rfultz commented May 6, 2021

Summary

Required reviewers

Impacted areas of the application

Screenshots

Related PRs

How to test

patphongs left a comment

Choose a reason for hiding this comment

patphongs commented May 17, 2021

patphongs left a comment

Choose a reason for hiding this comment

johnnyporkchops left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyporkchops May 18, 2021 • edited Loading

Choose a reason for hiding this comment

johnnyporkchops commented May 18, 2021 • edited Loading

johnnyporkchops commented May 19, 2021 • edited Loading

codecov-commenter commented May 20, 2021

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patphongs commented Jun 7, 2021

patphongs left a comment

Choose a reason for hiding this comment

johnnyporkchops left a comment •

edited

Loading

johnnyporkchops May 18, 2021 •

edited

Loading

johnnyporkchops commented May 18, 2021 •

edited

Loading

johnnyporkchops commented May 19, 2021 •

edited

Loading