harvesthq / platform

A whole new way to add time tracking to your web application.
https://www.getharvest.com/platform
59 stars 11 forks source link

Use async attribute. #35

Closed braddunbar closed 7 years ago

braddunbar commented 8 years ago

@adunkman @pusewicz

The async attribute is supported everywhere Harvest Platform is supported. Whatcha think?

pusewicz commented 8 years ago

I think that's fine? Nothing comes to my mind, so I'll give 1/2 of a đź‘Ť

adunkman commented 8 years ago

Hm, the script tag already uses the async attribute before this PR though.

pusewicz commented 8 years ago

Correct, but this is a more concise way of doing it? A single line versus multiple JS lines of code doing the same thing.

braddunbar commented 8 years ago

Correct, but this is a more concise way of doing it?

Right! I should've said that above. :) Injecting the script is a fallback for browsers that don't support async, right?

adunkman commented 8 years ago

Injecting the script is a fallback for browsers that don't support async, right?

I don’t believe so — I believe browsers that don’t support async will ignore the attribute, regardless of how it is set (but feel free to prove me wrong!).

I assume the script tag is injected programmatically for the same reason that I assume Google Analytics does it — it’s fewer things to ensure are copy and pasted correctly from the documentation. Either the tag works, or it doesn’t — there’s no partial success possibility.

For reference, Google Analytics’ code:

<script>
  (function(i,s,o,g,r,a,m){i['GoogleAnalyticsObject']=r;i[r]=i[r]||function(){
  (i[r].q=i[r].q||[]).push(arguments)},i[r].l=1*new Date();a=s.createElement(o),
  m=s.getElementsByTagName(o)[0];a.async=1;a.src=g;m.parentNode.insertBefore(a,m)
  })(window,document,'script','https://www.google-analytics.com/analytics.js','ga');

  ga('create', 'UA-xxxxxxxx-1', 'auto');
  ga('send', 'pageview');

</script>
braddunbar commented 7 years ago

Sorry for the late response here…doing some clean up of old pulls/issues!

I don’t believe so — I believe browsers that don’t support async will ignore the attribute, regardless of how it is set (but feel free to prove me wrong!).

Oh…I thought injecting it was effectively equivalent. Otherwise why not just include it as <script async src="…">?

I assume the script tag is injected programmatically for the same reason that I assume Google Analytics does it — it’s fewer things to ensure are copy and pasted correctly from the documentation. Either the tag works, or it doesn’t — there’s no partial success possibility.

Google Analytics also provides an alternative script:

While the JavaScript tracking snippet described above ensures the script will be loaded and executed asynchronously on all browsers, it has the disadvantage of not allowing modern browsers to preload the script.

The alternative async tracking snippet below adds support for preloading, which will provide a small performance boost on modern browsers, but can degrade to synchronous loading and execution on IE 9 and older mobile browsers that do not recognize the async script attribute. Only use this tracking snippet if your visitors primarily use modern browsers to access your site.

<script>
window.ga=window.ga||function(){(ga.q=ga.q||[]).push(arguments)};ga.l=+new Date;
ga('create', 'UA-XXXXX-Y', 'auto');
ga('send', 'pageview');
</script>
<script async src='https://www.google-analytics.com/analytics.js'></script>
braddunbar commented 7 years ago

Thanks for taking another look @adunkman! ❤️