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.

Sign up for GitHub

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

Jump to bottom

WIP: Removing render-blocking javascript #2300

Closed
wants to merge 2 commits into from
Closed

WIP: Removing render-blocking javascript #2300

wants to merge 2 commits into from

Conversation

contolini
Copy link
Member

@contolini contolini commented Aug 11, 2016 •
edited

One of cf.gov's biggest performance faux pas is loading jquery in the <head>. After exploring all sorts of ways to mitigate this (including exporting all the GTM snippets in an attempt to rewrite them all without jQuery), I finally tried just removing it from the head and added it to the common.js bundle.

And, surprisingly, it seems to work fine. I'm still in the process of testing this across all devices but so far everything appears to be working as expected.

Is this a horrible idea @sebworks @virginiacc? Have I overlooked something?

@jimmynotjim
Copy link
Contributor

Awesome. I think we tried this and GTM barfed, but maybe something has been updated there to make it work?

@KimberlyMunoz
Copy link
Contributor

I think the way we consistently got the problem is testing on slow devices. If you test in Chrome Developer tools with 2G speeds, then GA would load before our JS and cause errors.

@anselmbradford
Copy link
Member

Yeah, per @jimmynotjim's comment, IIRC we moved jquery out of the head (and removed it entirely on pages that didn't use it) and it broke the GTM code that assumes jquery is in the head when it executes. The larger problem that would be great to address is that the GTM code should be moved to version control (maybe even this repo). We essentially have JavaScript introduced by GTM that circumvents all our code review process.

@contolini
Copy link
Member Author

@KimberlyMunoz Good to know, I'll look into that.

@anselmbradford AFAIK GTM is not conducive to external version control, it would involve a lot of copying and pasting. It has its own versioning system that keeps a snapshot of every version of our GTM container.

Here's a dump of all the GTM snippets for the curious: https://[GHE]/raw/gist/contolini/a2bcfd2426611553ffd608e5f8f26da5/raw/b9c251ac7286ded2eae24146aa3204bfb21bca16/cfpb-gtm.json If you search for $ there are ~ 200 occurrences.

@anselmbradford
Copy link
Member

@anselmbradford AFAIK GTM is not conducive to external version control, it would involve a lot of copying and pasting. It has its own versioning system that keeps a snapshot of every version of our GTM container.

Chuck looked into injecting scripts directly into the object GTM adds to the page, potentially making it so the code could live here and not in GTM @wernerc where was the thread where you discussed that?

@sebworks
Copy link
Contributor

I think this explains it in detail:

#496

@sebworks
Copy link
Contributor

sebworks commented Aug 12, 2016 •
edited

How about this:

<noscript><iframe src="//www.googletagmanager.com/ns.html?id=GTM-KMMLRS"
height="0" width="0" style="display:none;visibility:hidden"></iframe></noscript>

<script>
    window.loadGTM = function loadGTM() {
        (function(w,d,s,l,i){w[l]=w[l]||[];w[l].push({'gtm.start':
        new Date().getTime(),event:'gtm.js'});var f=d.getElementsByTagName(s)[0],
        j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src=
        '//www.googletagmanager.com/gtm.js?id='+i+dl;f.parentNode.insertBefore(j,f);
        })(window,document,'script','dataLayer','GTM-KMMLRS');

        delete window.loadGTM;
    }
</script>

<script src="//ajax.googleapis.com/ajax/libs/jquery/1.11.3/jquery.min.js" onload="window.loadGTM()" async="true" ></script>
<script>if (!window.jQuery) { document.write('<script src="{{ static('js/jquery.min.js') }}" window.onload="loadGTM()" async="true"><\/script>'); }
</script>

This ensures that jQuery is loaded before loading GTM and adds the async tag to the jQuery scripts. This might break V1 scripts in some cases because we are still dependent upon jQuery is some places. Hopefully that will change soon.

@mistergone
Copy link
Member

These issues with GTM looking for jQuery on page load should be resolved (GTM should no longer barf on page load). There might be some specific pages where this still occurs, but I'm not currently aware of any (I think?). If this issue comes up again, or if someone sees a console error coming from GTM, please let me know!

@contolini
Copy link
Member Author

Thanks for the feedback, everyone! Super helpful. After chatting with folks, reading through the previous threads, and testing several script variants, I'm going to pause work on this for now. The odds of introducing a breaking change are high and the performance benefit doesn't justify the risk. We can reapproach it after the CF atomic stuff has wrapped up. Or if someone smarter than I wants to take a stab at it, please do.

@contolini contolini closed this Aug 23, 2016
@jimmynotjim jimmynotjim deleted the jquery-down-below branch November 1, 2017 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@contolini contolini

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@contolini @jimmynotjim @KimberlyMunoz @anselmbradford @sebworks @mistergone